Skip to content

feat(dialog, drawer): add onOpenChange details with reason and event#1198

Merged
te6-in merged 22 commits intodevfrom
feat/on-open-change-reason
Feb 3, 2026
Merged

feat(dialog, drawer): add onOpenChange details with reason and event#1198
te6-in merged 22 commits intodevfrom
feat/on-open-change-reason

Conversation

@te6-in
Copy link
Member

@te6-in te6-in commented Feb 2, 2026

Summary

  • Dialog와 Drawer의 onOpenChange 콜백에 details 객체를 추가하여 열림/닫힘 이유와 원본 이벤트를 제공합니다.
  • details.reason: 상태 변경 이유
  • details.event: 원본 네이티브 이벤트 (reason에 따라 타입이 narrowing됨)

Changes

@seed-design/react-dialog (headless)

  • DialogReasonToEventMap으로 reason별 이벤트 타입 매핑
  • DialogChangeDetails discriminated union 타입 추가
  • onOpenChange 콜백에 details 전달
  • reason: "trigger" | "closeButton" | "escapeKeyDown" | "interactOutside"

@seed-design/react-headless-drawer

  • DrawerReasonToEventMap으로 reason별 이벤트 타입 매핑
  • DrawerChangeDetails discriminated union 타입 추가
  • onOpenChange 콜백에 details 전달
  • reason: "closeButton" | "escapeKeyDown" | "interactOutside" | "drag" | "handleClickOnLastSnapPoint"

@seed-design/react (styled)

  • DialogActionDialogPrimitive.CloseButton으로 단순화

@seed-design/react-use-controllable-state

  • useControllableState에 제네릭 meta 파라미터 지원 추가

Documentation

  • Dialog, BottomSheet, MenuSheet onOpenChange details 문서 및 예제 추가

Test plan

  • Dialog 열기/닫기 시 onOpenChange에 올바른 reason이 전달되는지 확인
  • BottomSheet 열기/닫기 시 onOpenChange에 올바른 reason이 전달되는지 확인
  • ESC 키, 외부 클릭, 드래그 등 다양한 닫기 방식에서 reason이 정확한지 확인
  • TypeScript에서 reason 체크 시 event 타입이 narrowing되는지 확인

🤖 Generated with Claude Code

Summary by CodeRabbit

  • 새로운 기능

    • onOpenChange 콜백에 두 번째 인자로 상세한 reason 및 이벤트 컨텍스트 전달 지원(예: trigger, closeButton, escapeKeyDown, interactOutside, drag).
  • 문서

    • AlertDialog, MenuSheet, BottomSheet 등에서 onOpenChange 상세 이유와 예제(열기/닫기 이유 표시, prevent-close 데모) 추가/확장.
  • 테스트

    • 제어형 상태 훅에 대한 포괄적 테스트 스위트 추가.
  • 기타

    • Dialog 액션 단순화 및 닫기 동작 관련 일관성 개선.

@changeset-bot
Copy link

changeset-bot bot commented Feb 2, 2026

🦋 Changeset detected

Latest commit: e022954

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@seed-design/react-use-controllable-state Major
@seed-design/react-dialog Patch
@seed-design/react-drawer Patch
@seed-design/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 2, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Dialog/Drawer/BottomSheet/MenuSheet 등에서 onOpenChange 콜백 시그니처가 (open, details?)로 확장되어 두번째 인자(details: reason + event)를 전달하도록 변경되었고, 관련 문서·예제, useControllableState 구현 및 테스트가 추가되었으며 일부 컴포넌트 호출부가 이벤트-aware로 업데이트되었습니다.

Changes

Cohort / File(s) Summary
문서: 컴포넌트 설명 및 예제 추가
docs/content/react/components/alert-dialog.mdx, docs/content/react/components/menu-sheet.mdx, docs/content/react/components/bottom-sheet.mdx
onOpenChange의 두번째 인자(details)와 가능한 reason 값들(trigger, closeButton, escapeKeyDown, interactOutside, drag 등)을 문서에 추가하고 관련 예제 컴포넌트 참조를 삽입(일부 중복 섹션 포함).
문서 예제 파일
docs/examples/react/alert-dialog/prevent-close.tsx, docs/examples/react/alert-dialog/open-change-reason.tsx, docs/examples/react/menu-sheet/open-change-reason.tsx, docs/examples/react/bottom-sheet/open-change-reason.tsx
AlertDialog의 preventDefault로 닫기 방지 예제 및 여러 컴포넌트의 open/close reason 추적 예제 파일 추가.
헤드리스: useControllableState 패키지 도입·노출 및 테스트
packages/react-headless/use-controllable-state/src/useControllableState.tsx, packages/react-headless/use-controllable-state/src/index.ts, packages/react-headless/use-controllable-state/tsconfig.json, packages/react-headless/use-controllable-state/src/useControllableState.test.tsx
완전한 useControllableState 구현 추가(제어/비제어, details 지원, 모드 전환 경고), 패키지 루트 재노출 및 포괄적 테스트 추가.
헤드리스: Dialog 변경
packages/react-headless/dialog/src/useDialog.ts, packages/react-headless/dialog/src/Dialog.tsx
onOpenChange 시그니처를 (open, details?)로 변경하고 Trigger/Close 경로에서 reasonevent 전달; onDismiss 관련 주석 정리.
헤드리스: Drawer 및 드래그 흐름 변경
packages/react-headless/drawer/src/Drawer.tsx, packages/react-headless/drawer/src/use-snap-points.ts, packages/react-headless/drawer/src/useDrawer.ts
close/상태 변경 호출에 reasonevent 세부정보 포함하도록 시그니처·호출 방식 변경(escape, interactOutside, closeButton, drag 및 핸들 관련 경로 포함). useSnapPoints/onRelease에 이벤트 인자 추가.
react 컴포넌트: DialogAction 단순화
packages/react/src/components/Dialog/Dialog.tsx
로컬 forwardRef 기반 DialogAction 제거, DialogPrimitive.CloseButton의 직접 별칭으로 대체(공개 API 표면 변경).
임포트 경로 및 재노출
packages/react-headless/dialog/src/useDialog.ts, packages/react-headless/use-controllable-state/src/index.ts
useControllableState 임포트 경로를 @seed-design/react-use-controllable-state로 변경하고 패키지 루트에서 재노출함.

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant UI as UI\n(Trigger / CloseButton / Pointer)
    participant Headless as Headless\n(useControllableState / useDialog / useDrawer)
    participant Consumer as Consumer\n(onOpenChange)

    User->>UI: 클릭 / 키 입력 / 밖 클릭 / 드래그
    UI->>Headless: setOpen(true|false, { reason: "trigger"|"closeButton"|"escapeKeyDown"|"interactOutside"|"drag", event })
    Headless->>Consumer: onOpenChange(open, { reason, event })
    Consumer-->>Headless: (선택) 상태 동기화 또는 추가 로직
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🐇 문에 이유를 달아 전해요,
트리거 톡, 버튼 쿵, 바깥 스침도요.
훅은 이유와 이벤트를 안고 달려,
문서와 예제에 살포시 놓아두네.
당근 축하, 깡충깡충 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.52% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Pull request 제목은 주요 변경 사항인 onOpenChange에 details(reason과 event) 추가를 명확하게 요약하고 있습니다.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/on-open-change-reason

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 2, 2026

Deploying seed-design-v3 with  Cloudflare Pages  Cloudflare Pages

Latest commit: 47a12fe
Status: ✅  Deploy successful!
Preview URL: https://4ee18f51.seed-design.pages.dev
Branch Preview URL: https://feat-on-open-change-reason.seed-design.pages.dev

View logs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/react/src/components/Dialog/Dialog.tsx (1)

101-105: ⚠️ Potential issue | 🟡 Minor

DialogActionProps가 DialogPrimitive.CloseButtonProps와 불일치합니다

DialogAction이 DialogPrimitive.CloseButton으로 직접 alias되지만 DialogActionPropsDialogPrimitive.CloseButtonProps를 상속하지 않아 타입 일관성이 깨집니다. ActionSheet, MenuSheet, ExtendedActionSheet 등 다른 컴포넌트들은 모두 extends DialogPrimitive.CloseButtonProps 패턴을 따르고 있으니 Dialog도 동일하게 맞춰야 합니다.

제안 수정안
-export interface DialogActionProps
-  extends PrimitiveProps,
-    React.HTMLAttributes<HTMLButtonElement> {}
+export interface DialogActionProps extends DialogPrimitive.CloseButtonProps {}
🤖 Fix all issues with AI agents
In `@docs/content/react/components/alert-dialog.mdx`:
- Line 173: The text currently mentions "AlertDialogTrigger" but the
parenthetical uses "Dialog.Trigger", causing an inconsistent component
reference; update the parenthetical to match the earlier mention by replacing
"Dialog.Trigger" with the consistent identifier (either "AlertDialog.Trigger" or
"AlertDialogTrigger") so both usages refer to the same component name
(references to AlertDialogTrigger, AlertDialog.Trigger, and Dialog.Trigger
appear in the line—ensure only the correct AlertDialog variant is used).

In
`@packages/react-headless/use-controllable-state/src/useControllableState.test.tsx`:
- Around line 21-28: Tests call userEvent.click() without awaiting its Promise,
causing flakiness; update all test cases in useControllableState.test.tsx that
call userEvent.click (including the one rendering ControlledComponent and the
other similar checkbox/toggle tests) to prefix those calls with await, e.g.,
await userEvent.click(...), and keep subsequent assertions inside waitFor as
before to ensure events complete before assertions.
🧹 Nitpick comments (5)
docs/examples/react/alert-dialog/open-change-reason.tsx (1)

24-28: 매개변수 이름이 PR 목표와 일치하지 않습니다.

PR 목표에 따르면 metadetails로 이름이 변경되었습니다. 문서 예제에서도 일관성을 위해 details를 사용해야 합니다.

또한, Line 27의 삼항 연산자를 사용한 함수 호출 패턴은 간결하지만 가독성이 떨어질 수 있습니다.

♻️ 권장 수정안
-        onOpenChange={(open, meta) => {
+        onOpenChange={(open, details) => {
           setOpen(open);
 
-          (open ? setOpenReason : setCloseReason)(meta?.reason ?? null);
+          if (open) {
+            setOpenReason(details?.reason ?? null);
+          } else {
+            setCloseReason(details?.reason ?? null);
+          }
         }}
docs/examples/react/menu-sheet/open-change-reason.tsx (1)

22-26: alert-dialog 예제와 동일한 매개변수 이름 불일치.

meta 대신 details를 사용하여 PR의 명명 변경 사항과 일치시켜야 합니다.

♻️ 권장 수정안
-        onOpenChange={(open, meta) => {
+        onOpenChange={(open, details) => {
           setOpen(open);
 
-          (open ? setOpenReason : setCloseReason)(meta?.reason ?? null);
+          if (open) {
+            setOpenReason(details?.reason ?? null);
+          } else {
+            setCloseReason(details?.reason ?? null);
+          }
         }}
packages/react-headless/use-controllable-state/src/index.ts (1)

1-1: 타입 export 누락 확인 필요

useControllableState.tsx에서 ChangeHandler, SetStateFn, UseControllableStateParams 타입들도 export되고 있습니다. 외부 소비자들이 onChange 핸들러나 파라미터의 타입을 지정할 때 유용할 수 있으므로, 이 타입들도 re-export하는 것이 좋을 것 같습니다.

♻️ 타입 re-export 제안
-export { useControllableState } from "./useControllableState";
+export {
+  useControllableState,
+  type ChangeHandler,
+  type SetStateFn,
+  type UseControllableStateParams,
+} from "./useControllableState";
packages/react-headless/dialog/src/useDialog.ts (1)

31-38: 타입 표현 간소화 고려

Parameters<NonNullable<UseDialogStateProps["onOpenChange"]>>[1] 대신 DialogChangeDetails를 직접 사용하면 가독성이 향상될 수 있습니다.

♻️ 타입 간소화 제안
 function useDialogState(props: UseDialogStateProps) {
-  const [open = false, onOpenChange] = useControllableState<
-    boolean,
-    Parameters<NonNullable<UseDialogStateProps["onOpenChange"]>>[1]
-  >({
+  const [open = false, onOpenChange] = useControllableState<boolean, DialogChangeDetails>({
     prop: props.open,
     defaultProp: props.defaultOpen ?? false,
     onChange: props.onOpenChange,
   });
packages/react-headless/use-controllable-state/src/useControllableState.tsx (1)

57-70: value !== prop 참조 동등성 체크는 객체/배열 값에 문제를 야기할 수 있음

Line 61의 참조 동등성 체크는 React의 표준 Object.is 의미론을 따르지만, 객체나 배열을 값으로 사용하는 경우 의도치 않은 동작이 발생합니다. 부모 컴포넌트가 새로운 참조로 같은 값을 전달하면 onChange가 호출되지 않거나, 반대로 내용은 동일하지만 참조가 다르면 불필요하게 호출될 수 있습니다.

현재 구현이 Radix UI에서 파생되었으나, 다음을 권장합니다:

  • 즉시 조치: JSDoc으로 primitive 값 또는 불변 업데이트 패턴 사용을 명시
  • 개선 사항: Chakra UI의 shouldUpdate 패턴처럼 선택적 비교 함수 파라미터 추가를 고려 (사용자가 객체/배열 값으로 작업할 때 커스텀 동등성 로직 제공 가능)

@te6-in te6-in force-pushed the feat/on-open-change-reason branch from 1113c54 to bb3e34f Compare February 2, 2026 10:32
@te6-in te6-in changed the title feat(dialog): add onOpenChange details with reason and event feat(dialog, drawer): add onOpenChange details with reason and event Feb 2, 2026
@te6-in
Copy link
Member Author

te6-in commented Feb 2, 2026

#1202 이후 react-drawer reason: "trigger" 지원

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@docs/content/react/components/bottom-sheet.mdx`:
- Around line 106-111: Add the missing close reason "handleClickOnLastSnapPoint"
to the BottomSheet docs: update the 닫힐 때 list in the BottomSheet component docs
to include a bullet for "handleClickOnLastSnapPoint": 핸들 클릭으로 닫힘 (마지막 스냅 포인트에서),
so consumers see that BottomSheet (Drawer-based) can close when the handle is
clicked at the last snap point; ensure the entry appears alongside existing
reasons like "closeButton", "escapeKeyDown", "interactOutside", and "drag".

In `@packages/react-headless/dialog/src/Dialog.tsx`:
- Around line 64-83: Add defensive checks for event.defaultPrevented inside the
DismissableLayer handlers: in the onEscapeKeyDown handler (used with
api.closeOnEscape and api.setOpen) return early if event.defaultPrevented is
true before calling api.setOpen, and likewise in onInteractOutside (used with
api.closeOnInteractOutside and api.setOpen) return early if
event.defaultPrevented is true (use the originalEvent on event.detail when
checking). Ensure these checks happen before preventing default or changing open
state so user-provided handlers can cancel the built-in behavior.

In `@packages/react-headless/drawer/src/Drawer.tsx`:
- Around line 263-275: The current handlers call closeDrawer before giving user
handlers a chance to call preventDefault; update onInteractOutside and
onEscapeKeyDown to invoke props.onInteractOutside/props.onEscapeKeyDown first,
then check event.defaultPrevented (and dismissible/closeOn* flags) and only then
call closeDrawer; mirror the onPointerDownOutside pattern and for
onInteractOutside keep using event.detail.originalEvent when passing the event
to closeDrawer.
🧹 Nitpick comments (4)
packages/react-headless/use-controllable-state/src/useControllableState.test.tsx (1)

202-304: 컴포넌트 props는 interface로 분리해 주세요.

테스트 컴포넌트의 props가 인라인 타입 리터럴로 선언되어 있어 가독성과 재사용성이 떨어집니다. interface로 분리하면 가이드라인과 일관됩니다.
As per coding guidelines: prefer interfaces over types.

♻️ 리팩터링 예시
+interface ControlledToggleWithDetailsProps {
+  onChange: (value: boolean, details?: string) => void;
+  defaultValue?: boolean;
+}
+
+interface UncontrolledToggleWithDetailsProps {
+  onChange: (value: boolean, details?: string) => void;
+}
+
+interface ControlledComponentProps {
+  defaultChecked?: boolean;
+}
+
+interface UncontrolledComponentProps {
+  defaultChecked?: boolean;
+}
+
+interface CheckboxProps {
+  checked?: boolean;
+  defaultChecked?: boolean;
+  onChange?: (value: boolean) => void;
+}
+
 function ControlledToggleWithDetails({
   onChange,
   defaultValue = false,
-}: {
-  onChange: (value: boolean, details?: string) => void;
-  defaultValue?: boolean;
-}) {
+}: ControlledToggleWithDetailsProps) {
   const [value, setValue] = React.useState(defaultValue);
   const [, setValueWithDetails] = useControllableState<boolean, string>({
     prop: value,
     defaultProp: false,
     onChange: (newValue, details) => {
       setValue(newValue);
       onChange(newValue, details);
     },
   });
   ...
 }
 
-function UncontrolledToggleWithDetails({
-  onChange,
-}: {
-  onChange: (value: boolean, details?: string) => void;
-}) {
+function UncontrolledToggleWithDetails({ onChange }: UncontrolledToggleWithDetailsProps) {
   const [, setValue] = useControllableState<boolean, string>({
     defaultProp: false,
     onChange,
   });
   ...
 }
 
-function ControlledComponent({ defaultChecked }: { defaultChecked?: boolean }) {
+function ControlledComponent({ defaultChecked }: ControlledComponentProps) {
   const [checked, setChecked] = React.useState(defaultChecked ?? false);
   ...
 }
 
-function UncontrolledComponent({ defaultChecked }: { defaultChecked?: boolean }) {
+function UncontrolledComponent({ defaultChecked }: UncontrolledComponentProps) {
   return <Checkbox defaultChecked={defaultChecked} />;
 }
 
-function Checkbox(props: {
-  checked?: boolean;
-  defaultChecked?: boolean;
-  onChange?: (value: boolean) => void;
-}) {
+function Checkbox(props: CheckboxProps) {
   const [checked, setChecked] = useControllableState({
     defaultProp: props.defaultChecked ?? false,
     prop: props.checked,
     onChange: props.onChange,
     caller: "Checkbox",
   });
   ...
 }
docs/examples/react/bottom-sheet/open-change-reason.tsx (1)

23-27: 예제 코드의 가독성을 높이는 것이 좋습니다.

현재 삼항 연산자를 사용한 함수 호출 패턴은 기술적으로 유효하지만, 문서 예제로서 독자가 이해하기 어려울 수 있습니다.

♻️ 가독성 개선 제안
         onOpenChange={(open, details) => {
           setOpen(open);
 
-          (open ? setOpenReason : setCloseReason)(details?.reason ?? null);
+          if (open) {
+            setOpenReason(details?.reason ?? null);
+          } else {
+            setCloseReason(details?.reason ?? null);
+          }
         }}
packages/react-headless/dialog/src/useDialog.ts (1)

6-20: DialogChangeDetails.event를 필수로 두는 편이 타입 사용성이 좋습니다.

details가 전달될 때는 항상 원인 이벤트가 존재하므로 event? 때문에 소비자가 불필요한 undefined 분기를 처리해야 합니다. 가능하면 필수로 바꾸거나, 정말 undefined가 가능한 경우 주석에 명시해 주세요.

제안 변경
 type DialogChangeDetails = {
   [R in keyof DialogReasonToEventMap]: {
     /** The reason for the dialog open state change. */
     reason: R;
     /** The native event that triggered the change. */
-    event?: DialogReasonToEventMap[R];
+    event: DialogReasonToEventMap[R];
   };
 }[keyof DialogReasonToEventMap];
packages/react-headless/drawer/src/useDrawer.ts (1)

171-193: 불리언 변수명을 isOpen처럼 의미 있게 바꾸면 가독성이 좋아집니다.

o는 의도가 모호해 변경 이유/애니메이션 분기 가독성이 떨어집니다.

🛠️ 제안 변경
-    onChange: (o: boolean, details?: DrawerChangeDetails) => {
-      onOpenChange?.(o, details);
+    onChange: (isOpen: boolean, details?: DrawerChangeDetails) => {
+      onOpenChange?.(isOpen, details);
 
-      if (!o && !nested) {
+      if (!isOpen && !nested) {
         restorePositionSetting();
       }
 
       setTimeout(() => {
-        onAnimationEnd?.(o);
+        onAnimationEnd?.(isOpen);
       }, TRANSITIONS.EXIT_DURATION * 1000);
 
-      if (o && !modal) {
+      if (isOpen && !modal) {
         if (typeof window !== "undefined") {
           window.requestAnimationFrame(() => {
             document.body.style.pointerEvents = "auto";
           });
         }
       }
 
-      if (!o) {
+      if (!isOpen) {
         document.body.style.pointerEvents = "auto";
       }
     },

As per coding guidelines, Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).

te6-in and others added 14 commits February 2, 2026 19:58
Add optional meta parameter to useControllableState for passing
additional context through onChange callbacks. This enables use cases
like Dialog's onOpenChange(open, { reason }) to indicate why the
state changed (trigger, closeButton, escapeKeyDown, interactOutside).

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add DialogReasonToEventMap for type-safe event mapping
- Add DialogChangeDetails discriminated union type
- Pass reason and event to onOpenChange callback
- Update DismissableLayer handlers to include event details
- Switch from radix useControllableState to internal implementation

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Simplify DialogAction by delegating to primitive CloseButton which now
handles the onOpenChange details properly.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Rename meta to details in documentation
- Add details.event documentation
- Add open-change-reason and prevent-close examples

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@te6-in te6-in force-pushed the feat/on-open-change-reason branch from cfb0a9d to 6cb50c5 Compare February 2, 2026 11:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@docs/content/react/components/bottom-sheet.mdx`:
- Around line 96-109: Add the missing "trigger" open reason to the onOpenChange
documentation: update the `onOpenChange` section (referencing `onOpenChange` and
`reason`) to include the opening case with `reason: "trigger"` and a short
description (e.g., opened via trigger element or programmatic call), keeping
existing close reasons (`"closeButton"`, `"escapeKeyDown"`, `"interactOutside"`,
`"drag"`, `"handleClickOnLastSnapPoint"`) as-is for completeness.
🧹 Nitpick comments (1)
packages/react-headless/drawer/src/useDrawer.ts (1)

168-193: onChange 파라미터 이름을 더 설명적으로 바꿔 주세요.
o는 의미가 불분명해 유지보수 시 혼란이 생길 수 있습니다. isNextOpen처럼 의미가 드러나는 이름을 추천합니다.

♻️ 제안 변경
-    onChange: (o: boolean, details?: DrawerChangeDetails) => {
-      onOpenChange?.(o, details);
+    onChange: (isNextOpen: boolean, details?: DrawerChangeDetails) => {
+      onOpenChange?.(isNextOpen, details);

-      if (!o && !nested) {
+      if (!isNextOpen && !nested) {
         restorePositionSetting();
       }

       setTimeout(() => {
-        onAnimationEnd?.(o);
+        onAnimationEnd?.(isNextOpen);
       }, TRANSITIONS.EXIT_DURATION * 1000);

-      if (o && !modal) {
+      if (isNextOpen && !modal) {
         if (typeof window !== "undefined") {
           window.requestAnimationFrame(() => {
             document.body.style.pointerEvents = "auto";
           });
         }
       }

-      if (!o) {
+      if (!isNextOpen) {
         document.body.style.pointerEvents = "auto";
       }
     },

As per coding guidelines, Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).

@te6-in
Copy link
Member Author

te6-in commented Feb 2, 2026

/claude-review

@claude
Copy link

claude bot commented Feb 2, 2026

Claude finished @te6-in's task in 3m 39s —— View job


🤖 Claude Code Review 완료

📌 PR 이해

Dialog와 Drawer의 onOpenChange 콜백에 열림/닫힘의 구체적인 이유(reason)와 원본 이벤트(event)를 전달하는 기능을 추가했습니다. 새로운 @seed-design/react-use-controllable-state 패키지를 통해 메타데이터 전달을 지원하며, discriminated union 타입으로 타입 안전성을 확보했습니다.

✅ 리뷰 결과

🟢 전반적으로 매우 잘 구현되었습니다

이 PR은 견고한 설계와 구현을 보여줍니다:

강점

  • 타입 안전성: DialogReasonToEventMap / DrawerReasonToEventMap을 통한 discriminated union으로 reason별 이벤트 타입이 자동으로 narrowing됨
  • API 일관성: Dialog와 Drawer 모두 동일한 패턴 (onOpenChange(open, details)) 사용
  • 테스트 완비: useControllableState에 대한 포괄적인 테스트 케이스 포함
  • 문서 충실: 예제 코드와 함께 각 reason에 대한 명확한 설명 제공
  • CodeRabbit 피드백 반영: 이전 리뷰에서 지적된 사항들 (await 누락, 문서 불일치 등) 모두 수정됨

특히 좋은 점

  1. useControllableState의 제네릭 메타 파라미터 지원: 기존 Radix UI 패턴을 확장하여 상태 변경 시 추가 컨텍스트를 전달할 수 있게 함
  2. 이벤트 처리 순서: Drawer의 경우 사용자 핸들러를 먼저 실행한 후 defaultPrevented 체크하여 닫힘을 막을 수 있음 (CodeRabbit 피드백 반영)
  3. styled 컴포넌트 단순화: DialogActionDialogPrimitive.CloseButton으로 대체하여 불필요한 래퍼 제거

🟡 권장 개선사항 (선택적)

3개의 인라인 코멘트를 남겼습니다 (모두 선택적 제안):

  1. 타입 정의: event? optional 여부 검토 (현재도 좋지만 required도 고려 가능)
  2. 문서: BottomSheet 열림 reason 추가 (refactor: remove radix deps from react-drawer #1202 후 추가 예정이면 그때 반영)
  3. 예제 코드: 삼항 연산자 대신 if-else로 가독성 향상 (스타일 선호도)

💚 잘한 점

  1. 점진적 개선: 기존 API를 깨지 않고 두 번째 인자로 details를 추가
  2. Radix UI 패턴 준수: useInsertionEffect 활용, controlled/uncontrolled 전환 경고 등
  3. 철저한 테스트: 실제 사용 시나리오를 잘 반영한 테스트 케이스
  4. changeset 관리: react-use-controllable-state는 Major, 나머지는 Patch로 적절히 구분

🎯 결론

✅ Approve 추천

이 PR은 명확한 목적을 가지고 잘 설계되고 구현되었습니다. 위의 권장 사항들은 모두 선택적이며, 현재 상태로도 머지에 문제가 없습니다.

CodeRabbit의 피드백을 성실히 반영하여 코드 품질이 높고, 타입 안전성과 문서화가 잘 되어 있습니다. 특히 discriminated union을 활용한 타입 설계가 인상적입니다.


Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.changeset/busy-goats-count.md:
- Line 8: 변경 설명이 일부 컴포넌트만 언급되어 있어 범위가 불명확하므로 `.changeset/busy-goats-count.md`의
변경 설명을 업데이트해 모든 영향을 받는 컴포넌트를 명시하세요: `AlertDialogRoot`, `BottomSheetRoot` 뿐만 아니라
`Dialog`, `Drawer`, `MenuSheet`(및 PR에서 대상이라고 명시된 추가 컴포넌트)를 포함하고, 각 컴포넌트의
`onOpenChange` 두번째 인자로 전달되는 `details` 객체에서 사용할 수 있는 `details.reason`과
`details.event`를 명시해 사용자가 변경 범위와 사용법을 한눈에 알 수 있게 설명을 확장하세요.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@packages/react-headless/dialog/src/useDialog.ts`:
- Around line 6-11: DialogReasonToDetailMap currently mixes React synthetic
events (React.MouseEvent for trigger and closeButton) with native events
(KeyboardEvent, PointerEvent|FocusEvent); change the types on
DialogReasonToDetailMap so all reasons use native DOM event types (e.g.,
MouseEvent for trigger and closeButton, KeyboardEvent for escapeKeyDown,
PointerEvent|FocusEvent for interactOutside) and update any callers/handlers
(e.g., onTrigger/onCloseButton handlers) to pass e.nativeEvent where appropriate
(or document the design choice) so the public API is consistent; locate and
modify the DialogReasonToDetailMap interface and adjust usages in related
handlers/hooks (useDialog and any onTrigger/onCloseButton callback
implementations) to accept native events.

In `@packages/react-headless/drawer/src/useDrawer.ts`:
- Around line 17-23: DrawerReasonToDetailMap currently mixes React synthetic
event types and native event types; change all event types in
DrawerReasonToDetailMap to native DOM event types (e.g., MouseEvent,
PointerEvent, KeyboardEvent, FocusEvent) and update places that create
DrawerChangeDetails for onOpenChange to pass the native event (use
event.nativeEvent when starting from a React synthetic event). Specifically,
update the handlers that build the "drag", "closeButton", and
"handleClickOnLastSnapPoint" reasons in useDrawer (the code that constructs
DrawerChangeDetails and calls onOpenChange) to use the native event, and
propagate the same change to the drag-related callers in use-snap-points so they
pass native events rather than React synthetic events.
🧹 Nitpick comments (1)
packages/react-headless/drawer/src/useDrawer.ts (1)

442-449: 불리언 파라미터명을 보조동사 형태로 정리하면 가독성이 좋아집니다.
fromWithinisFromWithin 같은 형태로 바꾸는 것을 권장합니다.

♻️ 제안 수정안
-  const closeDrawer = useCallback(
-    (fromWithin?: boolean, details?: DrawerChangeDetails) => {
+  const closeDrawer = useCallback(
+    (isFromWithin?: boolean, details?: DrawerChangeDetails) => {
@@
-      if (!fromWithin) {
+      if (!isFromWithin) {
         setIsOpen(false, details);
       }
As per coding guidelines: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@packages/react-headless/drawer/src/useDrawer.ts`:
- Around line 17-30: The types and open-path aren’t including a "trigger" reason
so onOpenChange/onOpen isn’t given open semantics; add an entry "trigger: {
event: Event | undefined }" to DrawerReasonToDetailMap and regenerate
DrawerChangeDetails so the union includes reason: "trigger", then ensure the
code paths that open the drawer (the Trigger component handler and any
Root.onOpenChange/opening logic inside useDrawer) call the change/emit handler
with a detail object that sets reason: "trigger" and passes the originating
event (or undefined) so listeners receive the correct open reason.
🧹 Nitpick comments (1)
packages/react-headless/drawer/src/useDrawer.ts (1)

168-173: o 파라미터명을 isOpen으로 바꾸는 것을 권장합니다.

가독성과 규칙 준수 측면에서 불리합니다.

♻️ 제안 수정
-    onChange: (o: boolean, details?: DrawerChangeDetails) => {
-      onOpenChange?.(o, details);
+    onChange: (isOpen: boolean, details?: DrawerChangeDetails) => {
+      onOpenChange?.(isOpen, details);
 
-      if (!o && !nested) {
+      if (!isOpen && !nested) {
         restorePositionSetting();
       }
 
       setTimeout(() => {
-        onAnimationEnd?.(o);
+        onAnimationEnd?.(isOpen);
       }, TRANSITIONS.EXIT_DURATION * 1000);
 
-      if (o && !modal) {
+      if (isOpen && !modal) {
         if (typeof window !== "undefined") {
           window.requestAnimationFrame(() => {
             document.body.style.pointerEvents = "auto";
           });
         }
       }
 
-      if (!o) {
+      if (!isOpen) {
         document.body.style.pointerEvents = "auto";
       }
     },

As per coding guidelines, Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).

@te6-in te6-in merged commit 576c2e6 into dev Feb 3, 2026
6 of 7 checks passed
@te6-in te6-in deleted the feat/on-open-change-reason branch February 3, 2026 02:01
te6-in added a commit that referenced this pull request Feb 3, 2026
…1198)

* refactor: internalize useControllableState

* feat(use-controllable-state): add meta parameter support

Add optional meta parameter to useControllableState for passing
additional context through onChange callbacks. This enables use cases
like Dialog's onOpenChange(open, { reason }) to indicate why the
state changed (trigger, closeButton, escapeKeyDown, interactOutside).

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* refactor: meta → details

* feat(headless/dialog): add onOpenChange details with reason and event

- Add DialogReasonToEventMap for type-safe event mapping
- Add DialogChangeDetails discriminated union type
- Pass reason and event to onOpenChange callback
- Update DismissableLayer handlers to include event details
- Switch from radix useControllableState to internal implementation

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* refactor(react/dialog): use DialogPrimitive.CloseButton for DialogAction

Simplify DialogAction by delegating to primitive CloseButton which now
handles the onOpenChange details properly.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* docs(dialog): update onOpenChange documentation

- Rename meta to details in documentation
- Add details.event documentation
- Add open-change-reason and prevent-close examples

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* test: add missing await

* feat(headless/drawer): add onOpenChange details with reason and event

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* docs(bottom-sheet): add onOpenChange details documentation

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix(headless/drawer): remove unsupported trigger reason from type

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: fix deps

* docs

* refactor: make less diff

* docs

* fix: fix build

* docs: changeset

* docs

* refactor

* docs

* refactor: give native events to `onOpenChange`

* refactor: make reason optional

* docs

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
# Conflicts:
#	packages/react-headless/drawer/src/Drawer.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant