Conversation
Summary of ChangesHello @dioo1461, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 사용자가 채팅방에서 그룹 멤버 목록을 쉽게 확인할 수 있도록 새로운 기능을 도입합니다. '더보기' 버튼을 통해 액션 바텀 시트를 열고, 여기서 그룹 멤버 목록을 볼 수 있는 다이얼로그를 띄워 멤버들의 아바타와 닉네임을 표시합니다. 또한, 바텀 시트 컴포넌트의 유연성을 높이고 다이얼로그 컴포넌트의 스타일 정의를 개선하는 리팩토링도 포함되어 있습니다. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| }: ActionListBottomSheetProps) => ( | ||
| <BottomSheet {...props}> | ||
| {actionItems.map((item, idx) => ( | ||
| <View key={`actionButton-${item.label}`}> |
There was a problem hiding this comment.
리스트를 렌더링할 때 key는 각 항목을 고유하게 식별해야 합니다. 현재 item.label을 사용하고 있는데, 만약 actionItems 배열에 동일한 label을 가진 항목이 여러 개 존재할 경우 React가 항목을 제대로 구분하지 못해 예기치 않은 동작을 유발할 수 있습니다. key의 고유성을 보장하기 위해 idx를 함께 사용하는 것을 권장합니다.
| <View key={`actionButton-${item.label}`}> | |
| <View key={`actionButton-${item.label}-${idx}`}> |
| import Text from '@/components/Text'; | ||
| import { useDialog } from '@/hooks/useDialog'; | ||
|
|
||
| import { TITLE_TEXT_STYLE } from '../index.style'; |
There was a problem hiding this comment.
사용하지 않는 Text 컴포넌트와 TITLE_TEXT_STYLE 상수가 import되었습니다. 코드의 가독성과 유지보수성을 위해 불필요한 import는 제거하는 것이 좋습니다.
| import Text from '@/components/Text'; | |
| import { useDialog } from '@/hooks/useDialog'; | |
| import { TITLE_TEXT_STYLE } from '../index.style'; | |
| import { useDialog } from '@/hooks/useDialog'; |
| onPress: () => { | ||
| dialogService.add({ | ||
| content: ( | ||
| <UserInfosDialog userInfos={userInfos} /> | ||
| ), | ||
| }); | ||
| }, |
There was a problem hiding this comment.
멤버 목록 보기 버튼을 누르면 다이얼로그가 표시되는데, 이 때 뒤에 있는 BottomSheet가 닫히지 않아 사용자 경험을 해칠 수 있습니다. 다이얼로그를 표시하기 전에 setBottomSheetEnabled(false)를 호출하여 BottomSheet를 먼저 닫아주는 것이 좋겠습니다.
onPress: () => {
setBottomSheetEnabled(false);
dialogService.add({
content: (
<UserInfosDialog userInfos={userInfos} />
),
});
},
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2da8106cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,28 @@ | |||
| import { BottomSheet, Shapes } from '@components'; | |||
There was a problem hiding this comment.
Importing BottomSheet from @components here creates a circular dependency: ActionListBottomSheet → @components (barrel) → ./BottomSheet → ActionListBottomSheet. In Metro’s module initialization, this can leave the BottomSheet export undefined when ActionListBottomSheet first renders, leading to a runtime crash when the bottom sheet is opened. To avoid this, import BottomSheet from a local, non-barrel module (or split BottomSheet into a separate file) so the ActionListBottomSheet module doesn’t depend on the barrel that re-exports itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements a feature to view chat room members by adding a "more actions" button to the chat screen header. When clicked, a bottom sheet displays action options, including "View Group Members" which opens a dialog showing all members with their avatars and roles.
Changes:
- Added reusable
ActionListBottomSheetcomponent for displaying action menus in bottom sheets - Created
ChatMoreActionsBottomSheetandUserInfosDialogcomponents for the member list feature - Refactored Dialog style constants to a shared location for better code reusability
- Enhanced BottomSheet with
closeOnHardwareBackprop and automatic keyboard dismissal
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/screens/group/groupDetail/components/GroupColorPickerDialog/ColorPickItem/index.tsx | Removed trailing whitespace (cosmetic fix) |
| src/screens/group/GroupChatScreen/index.tsx | Integrated bottom sheet state and wired up user info data |
| src/screens/group/GroupChatScreen/components/UserInfosDialog/index.tsx | New dialog component displaying scrollable list of group members |
| src/screens/group/GroupChatScreen/components/UserInfosDialog/index.style.ts | Styles for member list dialog with scroll container |
| src/screens/group/GroupChatScreen/components/ScreenHeader/index.tsx | Added onMoreButtonPress prop to trigger actions menu |
| src/screens/group/GroupChatScreen/components/ChatMoreActionsBottomSheet/index.tsx | New component wrapping ActionListBottomSheet with member list action |
| src/locales/ko/groupChat.json | Added Korean translation for "group member list" |
| src/components/index.ts | Updated exports to include ActionListBottomSheet |
| src/components/Dialog/index.style.ts | Centralized shared Dialog style constants |
| src/components/Dialog/Confirm/index.tsx | Updated to import shared style constants |
| src/components/Dialog/Confirm/index.style.ts | Removed duplicated style constants |
| src/components/Dialog/Any/index.tsx | Added unused imports (needs cleanup) |
| src/components/Dialog/Any/index.style.ts | Removed centering styles from container (potential breaking change) |
| src/components/Dialog/Alert/index.tsx | Updated to import shared style constants |
| src/components/Dialog/Alert/index.style.ts | Removed duplicated style constants |
| src/components/BottomSheet/index.tsx | Changed to named exports and added ActionListBottomSheet export |
| src/components/BottomSheet/index.ios.tsx | Changed to named exports and added ActionListBottomSheet export |
| src/components/BottomSheet/index.android.tsx | Changed to named exports and added ActionListBottomSheet export |
| src/components/BottomSheet/BottomSheetCommon.tsx | Added closeOnHardwareBack prop and automatic keyboard dismissal |
| src/components/BottomSheet/ActionListBottomSheet/index.tsx | New reusable component for action lists in bottom sheets |
| src/components/BottomSheet/ActionListBottomSheet/index.style.ts | Styles for action button container |
| src/components/BottomSheet/ActionListBottomSheet/ActionButton.tsx | New component for individual action buttons with TouchableRipple |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <ChatMoreActionsBottomSheet | ||
| bottomSheetEnabled={isMoreActionsBottomSheetEnabled} | ||
| setBottomSheetEnabled={setIsMoreActionsBottomSheetEnabled} | ||
| userInfos={chatRoomInfoData.userInfos} |
There was a problem hiding this comment.
Remove trailing whitespace at the end of this line.
| userInfos={chatRoomInfoData.userInfos} | |
| userInfos={chatRoomInfoData.userInfos} |
| container: { | ||
| flex: 1, | ||
| alignItems: 'center', | ||
| container: { |
There was a problem hiding this comment.
The removal of flex: 1 and alignItems: 'center' from the Dialog.Any container style is a breaking change that could affect existing usages of Dialog.Any. While the existing usages in DateTimePickerWindows and GroupColorPickerDialog appear to handle their own layout, ensure that all usages of Dialog.Any throughout the codebase are tested to verify they still display correctly with this change.
| container: { | |
| container: { | |
| flex: 1, | |
| alignItems: 'center', |
| onClose={() => setBottomSheetEnabled(false)} | ||
| /> | ||
| ); | ||
|
|
There was a problem hiding this comment.
Remove the blank line between the closing JSX and the closing brace. This is inconsistent with the formatting in other components in the codebase.
| onPress: () => { | ||
| dialogService.add({ | ||
| content: ( | ||
| <UserInfosDialog userInfos={userInfos} /> | ||
| ), | ||
| }); | ||
| }, |
There was a problem hiding this comment.
The bottom sheet should be closed before opening the dialog. Currently, when the user presses the "View Group Members" action, the dialog opens while the bottom sheet remains visible in the background. This creates a poor user experience with overlapping UI elements. Add a call to setBottomSheetEnabled(false) before dialogService.add() to close the bottom sheet first.
| import Text from '@/components/Text'; | ||
| import { useDialog } from '@/hooks/useDialog'; | ||
|
|
||
| import { TITLE_TEXT_STYLE } from '../index.style'; |
There was a problem hiding this comment.
The imports Text and TITLE_TEXT_STYLE are not used in this component. These were likely added in preparation for a feature but are not actually utilized. Remove these unused imports to keep the code clean.
| import Text from '@/components/Text'; | |
| import { useDialog } from '@/hooks/useDialog'; | |
| import { TITLE_TEXT_STYLE } from '../index.style'; | |
| import { useDialog } from '@/hooks/useDialog'; |
| }; | ||
|
|
||
| export const USER_INFO_NICKNAME_TEXT_STYLE: TextProps = { | ||
| font: 'b3', |
There was a problem hiding this comment.
The USER_INFO_NICKNAME_TEXT_STYLE constant is exported but never imported or used anywhere in the codebase. Since the nickname text styling is applied inline with the font prop in UserInfosDialog/index.tsx line 28, this exported constant is redundant. Remove it to avoid confusion.
| }; | |
| export const USER_INFO_NICKNAME_TEXT_STYLE: TextProps = { | |
| font: 'b3', |
| }: ActionListBottomSheetProps) => ( | ||
| <BottomSheet {...props}> | ||
| {actionItems.map((item, idx) => ( | ||
| <View key={`actionButton-${item.label}`}> |
There was a problem hiding this comment.
Using item.label as part of the React key can cause issues if labels are not unique or if they change (e.g., when language changes in i18n). Consider using the array index as the key instead, or if actionItems have unique IDs, use those. Example: key={idx} or key={item.id || idx}.
| <View key={`actionButton-${item.label}`}> | |
| <View key={`actionButton-${idx}`}> |
| import { useEffect, useRef, useState } from 'react'; | ||
| import type { LayoutChangeEvent } from 'react-native'; | ||
| import { Animated, Dimensions, View } from 'react-native'; | ||
| import { Animated, Dimensions, Keyboard, View } from 'react-native'; |
There was a problem hiding this comment.
Unused import View.
| import { Animated, Dimensions, Keyboard, View } from 'react-native'; | |
| import { Animated, Dimensions, Keyboard } from 'react-native'; |
✨ 구현 기능 명세
✅ PR Point
😭 어려웠던 점