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은 네이티브 Time Picker 컴포넌트를 도입하여 시간 선택 기능을 개선하고, 전반적인 사용자 경험을 향상시키는 데 중점을 둡니다. 주요 의존성 업데이트와 폼 관련 컴포넌트의 리팩토링을 통해 코드의 안정성과 재사용성을 높였으며, Windows 플랫폼에서의 Time Picker 지원을 위한 설정도 포함되어 있습니다. 이를 통해 개발 효율성을 증대시키고, 다양한 플랫폼에서 일관된 기능을 제공할 수 있게 되었습니다. Highlights
Changelog
Activity
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
|
![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: def19ed2f0
ℹ️ 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".
| const dateToSeconds = (d: Date) => | ||
| d.getUTCHours() * 3600 + d.getUTCMinutes() * 60 + d.getUTCSeconds(); |
There was a problem hiding this comment.
The time picker returns a Date in the device’s local timezone, but dateToSeconds uses getUTCHours/getUTCMinutes, so in any non‑UTC timezone the stored seconds are shifted by the timezone offset (e.g., selecting 09:00 KST gets persisted as 00:00). This is compounded by secondsToDate constructing an epoch Date, which DateTimePicker will render in local time, so the displayed time is also offset. Use local accessors (getHours/getMinutes) and construct the Date in local time (e.g., new Date(0) + setHours) to keep the value consistent with what the user selected.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the @react-native-community/datetimepicker library and integrates it into the Form component for both Android and Windows platforms. The pre-commit hook was updated to include npx tsc --noEmit -p 'tsconfig.json' to perform type checking before commits. Several files were modified to implement the DateTimePicker, including platform-specific implementations (DateTimePicker.tsx, DateTimePickerWindows.tsx) and related types (types.ts). The Form component was enhanced with core components like HelperText and Label, and the Any component was updated to support these new features. The CategoryGoalTimeInput component was refactored to use the new DateTimePicker. The reviewer commented on the need to correct timezone handling in secondsToDate and dateToSeconds functions, suggesting the use of local time methods instead of UTC. Additionally, the reviewer suggested extracting inline styles in DateTimePickerDialog to a StyleSheet.create object for better maintainability and readability. The reviewer also suggested a type definition update for the children prop in AnyDialog to improve clarity.
| const secondsToDate = (seconds: number) => { | ||
| const s = Number.isFinite(seconds) ? seconds : 0; | ||
| return new Date(s * 1000); | ||
| }; | ||
|
|
||
| const dateToSeconds = (d: Date) => | ||
| d.getUTCHours() * 3600 + d.getUTCMinutes() * 60 + d.getUTCSeconds(); |
There was a problem hiding this comment.
secondsToDate와 dateToSeconds 함수에서 타임존 처리가 올바르지 않아 버그가 발생할 수 있습니다. new Date(milliseconds)는 UTC 기준으로 시간을 계산하므로, 로컬 타임존에 따라 의도치 않은 시간으로 표시될 수 있습니다. 또한 dateToSeconds에서 getUTCHours를 사용하면 사용자가 선택한 로컬 시간이 아닌 UTC 시간 기준으로 초를 계산하게 되어 값이 틀어질 수 있습니다. Date 객체의 로컬 시간 기준 메서드(getHours, setHours 등)를 사용하여 수정해야 합니다.
| const secondsToDate = (seconds: number) => { | |
| const s = Number.isFinite(seconds) ? seconds : 0; | |
| return new Date(s * 1000); | |
| }; | |
| const dateToSeconds = (d: Date) => | |
| d.getUTCHours() * 3600 + d.getUTCMinutes() * 60 + d.getUTCSeconds(); | |
| const secondsToDate = (seconds: number) => { | |
| const s = Number.isFinite(seconds) ? seconds : 0; | |
| const date = new Date(); | |
| date.setHours(Math.floor(s / 3600), Math.floor((s % 3600) / 60), s % 60, 0); | |
| return date; | |
| }; | |
| const dateToSeconds = (d: Date) => | |
| d.getHours() * 3600 + d.getMinutes() * 60 + d.getSeconds(); |
| <View style={{ | ||
| justifyContent: 'center', | ||
| alignItems: 'center', | ||
| marginVertical: 46, | ||
| backgroundColor: theme.color.neutral[500], | ||
| borderRadius: theme.radius[300], | ||
| }} | ||
| > | ||
| <DateTimePicker | ||
| {...props} | ||
| style={{ justifyContent: 'center', alignItems: 'center', width: 280, height: 32 }} | ||
| /> | ||
| </View> |
There was a problem hiding this comment.
DateTimePickerDialog 컴포넌트 내부에 인라인 스타일이 사용되고 있습니다. 코드 가독성, 유지보수성 및 성능을 위해 스타일을 StyleSheet.create를 사용하여 별도의 객체로 추출하는 것이 좋습니다. 파일 상단에 StyleSheet를 import하고, 파일 하단에 아래 styles 객체를 추가해주세요.
import { StyleSheet, View } from 'react-native';
// ...
const styles = StyleSheet.create({
dialogContent: {
justifyContent: 'center',
alignItems: 'center',
marginVertical: 46,
backgroundColor: theme.color.neutral[500],
borderRadius: theme.radius[300],
},
picker: {
justifyContent: 'center',
alignItems: 'center',
width: 280,
height: 32,
},
}); <View style={styles.dialogContent}>
<DateTimePicker
{...props}
style={styles.picker}
/>
</View>
src/components/Dialog/Any/index.tsx
Outdated
| import { styles } from './index.style'; | ||
|
|
||
| interface AnyDialogProps { | ||
| children: ({ closeDialog }: { closeDialog: () => void }) => ReactNode | ReactNode; |
There was a problem hiding this comment.
children prop의 타입 정의가 ReactNode | ReactNode를 포함하고 있어 혼동을 줄 수 있습니다. render prop 패턴을 명확하게 표현하기 위해 ReactNode | (({ closeDialog }: { closeDialog: () => void }) => ReactNode)와 같이 타입을 수정하면 가독성이 향상됩니다.
| children: ({ closeDialog }: { closeDialog: () => void }) => ReactNode | ReactNode; | |
| children: ReactNode | (({ closeDialog }: { closeDialog: () => void }) => ReactNode); |
There was a problem hiding this comment.
Pull request overview
Implements a Time Picker component by integrating @react-native-community/datetimepicker, wiring up Windows native module autolinking, and updating category “goal time” input to use the new picker. The PR also includes supporting refactors in Form/Dialog components, type tightening, and dependency/tooling updates.
Changes:
- Add a cross-platform
DateTimePickerwrapper (with a Windows dialog implementation) and use it for category goal time input. - Update Windows RNW project files to autolink/register the datetimepicker native module.
- Refactor Form element typings/implementations and update related screens/locales for the new APIs.
Reviewed changes
Copilot reviewed 53 out of 57 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| windows/studyPals/pch.h | Add WinRT header include for DateTimePicker. |
| windows/studyPals/packages.lock.json | Add datetimepickerwindows project dependency entry. |
| windows/studyPals/AutolinkedNativeModules.g.targets | Autolink DateTimePickerWindows VCXPROJ reference. |
| windows/studyPals/AutolinkedNativeModules.g.cpp | Include/register DateTimePicker React package provider via autolinking. |
| windows/studyPals/App.xaml | Force Windows app theme to Dark. |
| windows/studyPals/App.cpp | Append DateTimePicker package provider in app initialization. |
| windows/studyPals.sln | Add DateTimePicker project and update solution configuration sections. |
| src/utils/dialog/index.ts | Add dialogService.close() API. |
| src/types/form.d.ts | Make defaultValue, value, onValueChange required in form types. |
| src/screens/signUp/SignupAccountScreen/components/SignUpRequiredForm/index.tsx | Provide defaultValue props for required form inputs. |
| src/screens/signUp/SignupAccountScreen/components/SignUpRequiredForm/PasswordFields.tsx | Provide defaultValue props for password inputs. |
| src/screens/signUp/SignUpProfileImageScreen/index.tsx | Rename destructured mutation function to mutateAsync. |
| src/screens/group/GroupSearchScreen/components/ResultGroups/index.tsx | Replace hardcoded mock groups with an empty list. |
| src/screens/group/GroupChatScreen/__mock.ts | Remove chat mock data file. |
| src/screens/category/EditCategoryScreen/index.tsx | Formatting-only adjustments in CategoryGoalTimeInput usage. |
| src/screens/category/CreateCategoryScreen/index.tsx | Update default values to match stricter form typing. |
| src/screens/LoginScreen/index.tsx | Add defaultValue for login inputs due to type changes. |
| src/mocks/handlers/category.ts | Narrow mock studyType literal type with as const. |
| src/locales/ko/createCategory.json | Rename/extend goal time keys (label/value formatting) and fix trailing comma. |
| src/locales/ko/components.json | Add time wheel picker localization keys. |
| src/features/category/ui/CategoryGoalTimeInput/index.tsx | New goal time input using DateTimePicker + Form.Any render-prop. |
| src/features/category/ui/CategoryGoalTimeInput/index.style.ts | Styles for new goal time input display. |
| src/features/category/ui/CategoryGoalTimeInput.tsx | Remove old numeric TextInput-based goal time input. |
| src/features/category/ui/CategoryDayBelongInput/index.tsx | Update Form.Any usage to render-prop pattern and stricter types. |
| src/components/index.ts | Export new DateTimePicker component. |
| src/components/Form/types.ts | Introduce shared CommonFormElementProps for form elements. |
| src/components/Form/elements/index.style.ts | Add shared element container styles (gap). |
| src/components/Form/elements/TextInput.tsx | Refactor props typing + internal value conversion and RHF wiring. |
| src/components/Form/elements/SubmitButton.tsx | Simplify render (remove fragment wrapper). |
| src/components/Form/elements/CheckBox.tsx | Update props typing and minor handler naming. |
| src/components/Form/elements/Any.tsx | Convert Any element from cloneElement to render-prop; add label/hint/error UI. |
| src/components/Form/core/label.style.ts | New label styles (required marker). |
| src/components/Form/core/inputField.style.ts | New input field container style helper and border-color logic. |
| src/components/Form/core/Label.tsx | New Label component for Form core. |
| src/components/Form/core/HelperText.tsx | New HelperText component for Form core. |
| src/components/Dialog/index.tsx | Add Dialog.Any export. |
| src/components/Dialog/Any/index.tsx | Introduce generic dialog container with closeDialog helper. |
| src/components/Dialog/Any/index.style.ts | Styles for AnyDialog container. |
| src/components/DateTimePicker/types.ts | Define common DateTimePicker wrapper prop types. |
| src/components/DateTimePicker/index.windows.ts | Platform entrypoint for Windows implementation. |
| src/components/DateTimePicker/index.ts | Default platform entrypoint for DateTimePicker wrapper. |
| src/components/DateTimePicker/DateTimePickerWindows.tsx | Windows picker implemented via app Dialog + dialogService. |
| src/components/DateTimePicker/DateTimePicker.tsx | Default picker wrapper toggling mount/unmount via local state. |
| src/components/DatePicker/useDatePicker/useHighlightRange.ts | Remove highlight range hook (range feature). |
| src/components/DatePicker/useDatePicker/useDatePickerRange.ts | Remove date range selection hook (range feature). |
| src/components/DatePicker/index.tsx | Remove Range export wiring while leaving Select. |
| src/components/DatePicker/context/DatePickerRangeProvider.tsx | Remove provider for range date picker. |
| src/components/DatePicker/Table/Cell/DowCell.tsx | Tighten color prop type to Color. |
| src/components/DatePicker/Table/Cell/DateCell.tsx | Tighten style typing for date cell colors. |
| src/components/DatePicker/DatePickerRange.tsx | Remove DatePicker range component implementation. |
| src/components/Button/Basic/index.tsx | Tighten textColor type to Color. |
| src/components/Button/Basic/ButtonText.tsx | Tighten color prop type to Color and remove unused import. |
| src/components/BottomSheet/index.android.tsx | Remove unused imports. |
| package.json | Add datetimepicker dependency; bump TypeScript + typescript-eslint versions. |
| package-lock.json | Lockfile updates for new dependency and TS/eslint toolchain bump. |
| .github/hooks/pre-commit | Run tsc --noEmit in pre-commit hook. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RegisterAutolinkedNativeModulePackages(PackageProviders()); // Includes any autolinked modules | ||
|
|
||
| PackageProviders().Append(make<ReactPackageProvider>()); // Includes all modules in this project | ||
| PackageProviders().Append(winrt::SQLitePlugin::ReactPackageProvider()); | ||
| PackageProviders().Append(winrt::DateTimePicker::ReactPackageProvider()); | ||
| InitializeComponent(); |
There was a problem hiding this comment.
RegisterAutolinkedNativeModulePackages(PackageProviders()) already appends winrt::DateTimePicker::ReactPackageProvider() via AutolinkedNativeModules.g.cpp, so appending it again here will register the package twice. Remove the manual PackageProviders().Append(winrt::DateTimePicker::ReactPackageProvider()); line (or disable autolinking for this module) to avoid duplicate native module registration.
| import { Button, DateTimePicker, Form, Toast, TText } from '@/components'; | ||
| import { useTranslatedText } from '@/hooks/useTranslatedText'; | ||
| import type { FormElementProps } from '@/types'; |
There was a problem hiding this comment.
Unused imports (Button, Toast) should be removed to keep the module clean (and avoid failing stricter lint rules).
| const DatePicker = () => { | ||
| throw new Error('!!! DatePicker.Select, DatePicker.Range를 사용해주세요 !!!'); | ||
| }; |
There was a problem hiding this comment.
The thrown error message still mentions DatePicker.Range, but DatePicker.Range is no longer exported/assigned in this file. Update the message to reflect the supported API (or re-add Range if it’s intended to remain available).
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | ||
| xmlns:local="using:studyPals" | ||
| xmlns:react="using:Microsoft.ReactNative"> | ||
| <Application.RequestedTheme>Dark</Application.RequestedTheme> |
There was a problem hiding this comment.
This PR forces the entire Windows app theme to Dark. That’s a behavior change unrelated to the Time Picker feature described in the PR and may override user/system theme preferences. Consider reverting this, making it configurable, or moving it to a separate PR with rationale.
| import type DateTimePicker from '@react-native-community/datetimepicker'; | ||
| import type { ComponentProps, ReactNode } from 'react'; | ||
|
|
||
| export type CommonDateTimePickerProps = { | ||
| dateTimePickerProps: ComponentProps<typeof DateTimePicker>; | ||
| trigger: (open: () => void) => ReactNode; |
There was a problem hiding this comment.
import type DateTimePicker ... cannot be used with ComponentProps<typeof DateTimePicker> because typeof is a value query and DateTimePicker was imported as a type-only symbol. This will fail TypeScript compilation. Import the component as a value (non-type) or use the library’s exported props type (e.g. DateTimePickerProps) for dateTimePickerProps.
| {isOpen && | ||
| <RNDateTimePicker | ||
| {...dateTimePickerProps} | ||
| onChange={(event, selectedDate) => { | ||
| setIsOpen(false); | ||
| dateTimePickerProps.onChange?.(event, selectedDate); | ||
| }} | ||
| />} |
There was a problem hiding this comment.
The picker closes on the first onChange event (setIsOpen(false)), which breaks iOS display='spinner' (and other inline modes) because onChange fires continuously while scrolling. Only close on platforms/events where selection is finalized (e.g. Android event.type === 'set'/'dismissed'), or provide an explicit Done/Cancel action for inline pickers.
| if (dialogIdRef.current !== null) { | ||
| dialogService.close(dialogIdRef.current); | ||
| } | ||
| dialogIdRef.current = null; | ||
| dateTimePickerProps.onChange?.(event, selectedDate); |
There was a problem hiding this comment.
The dialog is closed immediately on the first onChange event. If the Windows picker fires onChange while the user is still adjusting (spinner/inline), this will close the dialog prematurely. Consider closing only on a definitive completion/dismiss signal (e.g. inspect event.type) or add explicit confirm/cancel controls in the dialog.
| if (dialogIdRef.current !== null) { | |
| dialogService.close(dialogIdRef.current); | |
| } | |
| dialogIdRef.current = null; | |
| dateTimePickerProps.onChange?.(event, selectedDate); | |
| // Forward all change events to the original handler | |
| dateTimePickerProps.onChange?.(event, selectedDate); | |
| // Only close the dialog on a definitive completion/dismiss signal | |
| if (event?.type === 'set' || event?.type === 'dismissed') { | |
| if (dialogIdRef.current !== null) { | |
| dialogService.close(dialogIdRef.current); | |
| } | |
| dialogIdRef.current = null; | |
| } |
| const secondsToDate = (seconds: number) => { | ||
| const s = Number.isFinite(seconds) ? seconds : 0; | ||
| return new Date(s * 1000); | ||
| }; | ||
|
|
There was a problem hiding this comment.
secondsToDate/dateToSeconds are currently using epoch-based Date plus UTC getters, which will produce timezone-shifted times (e.g. in KST, 0 seconds displays as 09:00, and selected 01:00 becomes 16:00 when read via getUTCHours). For a duration-like “seconds in day” value, build the Date in local time (e.g. start from new Date(0) and setHours(0,0,0,0); setSeconds(s)) and convert back with local getters (getHours()/getMinutes()), not UTC.
src/components/Dialog/Any/index.tsx
Outdated
| import { styles } from './index.style'; | ||
|
|
||
| interface AnyDialogProps { | ||
| children: ({ closeDialog }: { closeDialog: () => void }) => ReactNode | ReactNode; |
There was a problem hiding this comment.
children is typed only as a function, but the implementation also supports ReactNode (non-function) and has a runtime typeof children === 'function' check. Change the prop type to a proper union (e.g. ReactNode | (({closeDialog}) => ReactNode)) and remove the redundant ReactNode | ReactNode in the return type.
| children: ({ closeDialog }: { closeDialog: () => void }) => ReactNode | ReactNode; | |
| children: ReactNode | (({ closeDialog }: { closeDialog: () => void }) => ReactNode); |
| const dialogIdRef = useRef<string | null>(null); | ||
|
|
||
| const handleOpen = () => { | ||
| dialogIdRef.current = dialogService.add({ | ||
| content: ( | ||
| <DateTimePickerDialog | ||
| {...dateTimePickerProps} | ||
| onChange={(event, selectedDate) => { | ||
| if (dialogIdRef.current !== null) { | ||
| dialogService.close(dialogIdRef.current); | ||
| } | ||
| dialogIdRef.current = null; | ||
| dateTimePickerProps.onChange?.(event, selectedDate); | ||
| }} | ||
| /> | ||
| ), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
handleOpen always calls dialogService.add(...) without checking whether a picker dialog is already open (dialogIdRef.current). If the trigger is pressed multiple times, multiple dialogs can be stacked and the ref will only track the latest one. Consider guarding (no-op if already open), or closing the existing dialog before opening a new one, and/or clearing the ref via options.onDismissed when the dialog is dismissed by backdrop/back button.
…als/frontend into feature/date-time-picker
…als/frontend into feature/date-time-picker
✨ 구현 기능 명세
네이티브 UI를 연동하여 Time Picker 컴포넌트 구현했습니다.
✅ PR Point
😭 어려웠던 점