[FE-Feat] 홈 화면 API 연결 #213
Conversation
WalkthroughThis PR makes numerous enhancements to the frontend. It introduces new SVG icon components (CheckGraphic and ClockGraphic) and refactors the SegmentControl to use objects for options. The shared-schedule layer now features new API endpoints, query keys, prefetch functions, and custom query hooks, along with Zod schemas and type definitions for upcoming, ongoing, and finished schedules. Additionally, new UI components for displaying finished, ongoing, and upcoming schedules have been implemented while obsolete unconfirmed/expired components have been removed. Updates were also made to pagination, routing, date utilities, and styling. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Interface
participant OS as OngoingSchedules Component
participant SC as SegmentControl
participant OQL as useOngoingQuery Hook
participant API as schedulesApi
participant DS as ScheduleDetails Component
participant DQ as useDiscussionQuery Hook
U->>OS: Loads Ongoing Schedules screen
OS->>SC: Render segment options (All, Created, Shared)
SC->>OS: User selects a segment option
OS->>OQL: Call hook with current page & segment option
OQL->>API: Fetch ongoing schedules data
API-->>OQL: Return schedule data
OQL-->>OS: Provide schedule list
OS->>U: Render OngoingScheduleList with schedule items
U->>OS: Click on a schedule item
OS->>DQ: Call useDiscussionQuery with discussionId
DQ->>API: Fetch discussion details
API-->>DQ: Return discussion details
DQ-->>DS: Render detailed schedule info
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (3)
frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx (1)
43-49: 🛠️ Refactor suggestionAdd accessibility attributes to the chevron button.
The button element lacks essential accessibility attributes such as
aria-labelandrole.Apply this diff to improve accessibility:
- <button className={chevronButtonStyle({ latest })}> + <button + aria-label="View schedule details" + className={chevronButtonStyle({ latest })} + role="button" + >frontend/src/pages/UpcomingSchedulePage/index.tsx (1)
10-10:⚠️ Potential issueReplace mock data with real API data.
The component is using mock data (
[{}, {}, {}, {}, {}]) instead of real schedule data. Consider using the new schedule API hooks for data fetching.const UpcomingSchedulePage = () => { - const schedules = [{}, {}, {}, {}, {}]; + const { data: schedules } = useUpcomingQuery(); return ( <Flex className={containerStyle} direction='column' gap={700} > <UpcomingSchedules> <Text typo='h2'>다가오는 일정</Text> </UpcomingSchedules> - <UpcomingScheduleList schedules={schedules} /> + <UpcomingScheduleList schedules={schedules ?? []} /> </Flex> ); };Also applies to: 20-20
frontend/src/utils/date/time.ts (1)
20-24: 🛠️ Refactor suggestionRemove duplicate constant definition.
The
MINUTE_IN_MILLISECONDSconstant is now exported at the module level. Remove the local constant ingetMinuteDiffand use the exported one.Apply this diff to remove the duplicate constant:
export const getMinuteDiff = (startTime: Date, endTime: Date): number => { - const MINUTE_IN_MILLISECONDS = 60000; const diff = endTime.getTime() - startTime.getTime(); return Math.floor(diff / MINUTE_IN_MILLISECONDS); };
🧹 Nitpick comments (29)
frontend/src/features/shared-schedule/ui/Fallbacks/index.css.ts (1)
5-8: Consider using a more specific style name.While the style definition is clean and follows best practices by using theme variables, the name
containerStyleis quite generic. Since this is specifically for a Fallback component (as indicated in the PR context), consider using a more descriptive name.-export const containerStyle = style({ +export const fallbackContainerStyle = style({ backgroundColor: vars.color.Ref.CoolGrey[50], borderRadius: vars.radius[700], });frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx (1)
54-75: Consider optimizing the new subcomponents.The extracted components improve code organization. Consider these enhancements:
- Memoize components to prevent unnecessary re-renders
- Move to separate files if they grow in complexity
Example memoization:
-const DdayChip = ({ endDateTime, latest }: { +const DdayChip = React.memo(({ endDateTime, latest }: { endDateTime: Date; latest: boolean; -}) => ( +}) => ( <Chip color={latest ? 'black' : 'coolGray'} radius='max' size='md' style={latest ? 'filled' : 'weak'} > {formatDateToDdayString(endDateTime)} </Chip> -); +)); -const MeetingDateTimeInfo = ({ startDateTime, endDateTime }: { +const MeetingDateTimeInfo = React.memo(({ startDateTime, endDateTime }: { startDateTime: Date; endDateTime: Date; -}) => ( +}) => ( <Text color={vars.color.Ref.Netural[600]} typo='b2R'> {getDateTimeRangeString(startDateTime, endDateTime)} </Text> -); +));frontend/src/components/SegmentControl/ControlButton.tsx (1)
12-18: Consider memoizing the destructured values.While the current implementation is correct, consider memoizing the destructured values if they're used in multiple places or if the component re-renders frequently.
const ControlButton = ({ segmentOption, segmentControlStyle }: ControlButtonProps ) => { const { selectedValue, handleSelect } = useSafeContext(SegmentControlContext); - const { label, value } = segmentOption; + const { label, value } = useMemo(() => segmentOption, [segmentOption]);Also applies to: 27-27
frontend/src/components/SegmentControl/SegmentControl.stories.tsx (1)
31-43: Consider adding JSDoc comments to document the WithContent pattern.The WithContent story demonstrates a complex use case. Consider adding JSDoc comments to explain the relationship between
segmentOptionsandSegmentControl.Contentvalues.+/** + * Demonstrates using SegmentControl with content panels. + * Each SegmentControl.Content's value must match a value in segmentOptions. + */ export const WithContent = () => (frontend/src/features/discussion/ui/DiscussionRank/index.tsx (2)
61-64: Consider separating labels from internal values.Currently, the labels and values are identical. Consider using more maintainable internal values:
segmentOptions={[ - { label: '참가자 많은 순', value: '참가자 많은 순' }, - { label: '빠른 시간 순', value: '빠른 시간 순' }, + { label: '참가자 많은 순', value: 'most_participants' }, + { label: '빠른 시간 순', value: 'earliest_time' }, ]}
67-67: Update Content values to match the suggested internal values.If you implement the suggested value changes above, update the Content values accordingly:
-<SegmentControl.Content value={'참가자 많은 순'}> +<SegmentControl.Content value={'most_participants'}> -<SegmentControl.Content value={'빠른 시간 순'}> +<SegmentControl.Content value={'earliest_time'}>Also applies to: 70-70
frontend/src/components/SegmentControl/index.tsx (2)
27-27: Consider adding validation for empty segmentOptions.The current default value handling uses optional chaining but could benefit from explicit validation:
+if (segmentOptions.length === 0) { + throw new Error('SegmentControl requires at least one option'); +} const [selectedValue, setSelectedValue] = useState(defaultValue);
49-55: Consider using value as the key instead of composite key.Since values should be unique within the options, consider using just the value as the key:
{segmentOptions.map((segmentOption, idx) => ( <ControlButton - key={`${segmentOption.value}-${idx}`} + key={segmentOption.value} segmentControlStyle={style} segmentOption={segmentOption} /> ))}frontend/src/features/shared-schedule/ui/Fallbacks/FinishedFallback.tsx (2)
11-12: Consider using responsive dimensionsThe fixed dimensions (
height='41.375rem' width='83.25rem') might cause layout issues on different screen sizes. Consider using relative units or making the component more responsive.- height='41.375rem' - width='83.25rem' + height='100%' + width='100%' + minHeight='41.375rem' + maxWidth='83.25rem'
15-15: Move text to i18n/constants fileConsider moving the Korean text to a dedicated i18n or constants file for better maintainability and localization support.
- <Text color={vars.color.Ref.Netural[700]} typo='h3'>지난 일정이 없어요</Text> + <Text color={vars.color.Ref.Netural[700]} typo='h3'>{t('schedule.finished.empty')}</Text>frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1)
5-12: Add JSDoc comments for better documentationConsider adding JSDoc comments to describe the schema properties and their purposes.
+/** + * Schema for an upcoming schedule + * @property {number} id - Unique identifier for the schedule + * @property {string} title - Title of the schedule + * @property {Date} startDateTime - Start date and time + * @property {Date} endDateTime - End date and time + * @property {'ONLINE' | 'OFFLINE'} meetingMethodOrLocation - Meeting method + * @property {string[]} participantPictureUrls - List of participant picture URLs + */ const UpcomingScheduleSchema = z.object({ id: z.number(), title: z.string(), startDateTime: zCoerceToDate, endDateTime: zCoerceToDate, meetingMethodOrLocation: z.enum(['ONLINE', 'OFFLINE']), participantPictureUrls: z.array(z.string()), });frontend/src/features/shared-schedule/ui/Fallbacks/UpcomingFallback.tsx (1)
14-15: Consider using responsive dimensions.The hardcoded dimensions (
height='19.75rem'andwidth='83.25rem') might not adapt well to different screen sizes. Consider using relative units or responsive design patterns.- height='19.75rem' - width='83.25rem' + height='100%' + width='100%' + minHeight='19.75rem' + maxWidth='83.25rem'frontend/src/features/shared-schedule/api/prefetch.ts (1)
9-11: Consider parameterizing pagination values.The pagination parameters (page=1, size=10) are hardcoded. Consider making these configurable to support different use cases and improve reusability.
-export const prefetchOngoingSchedules = async (queryClient: QueryClient) => { - await queryClient.prefetchQuery(sharedSchedulesQueryOptions.ongoing(1, 10, 'ALL')); +export const prefetchOngoingSchedules = async ( + queryClient: QueryClient, + page = 1, + size = 10, +) => { + await queryClient.prefetchQuery(sharedSchedulesQueryOptions.ongoing(page, size, 'ALL')); }; -export const prefetchFinishedSchedules = async (queryClient: QueryClient) => { +export const prefetchFinishedSchedules = async ( + queryClient: QueryClient, + page = 1, + size = 10, +) => { await queryClient.prefetchQuery( - sharedSchedulesQueryOptions.finished(1, 10, new Date().getFullYear()), + sharedSchedulesQueryOptions.finished(page, size, new Date().getFullYear()), ); };Also applies to: 13-17
frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx (1)
17-23: Add error handling for empty schedules.The component doesn't handle the case when schedules array is empty. Consider adding a check or fallback UI.
+ if (!schedules.length) { + return <UpcomingFallback />; + } {schedules.map((schedule, index) => ( <ScheduleCard key={index} latest={false} schedule={schedule} /> ))}frontend/src/pages/UpcomingSchedulePage/index.css.ts (1)
7-13: Consider responsive design implications of fixed width.The fixed width of 1284px might cause horizontal scrolling on smaller screens. Consider using a responsive approach:
- Use max-width instead of width
- Add media queries for different screen sizes
Example improvement:
- width: 1284, + width: '100%', + maxWidth: 1284,frontend/src/features/shared-schedule/model/ongoingSchedules.ts (1)
5-12: Enhance validation for timeLeft field.The
timeLeftfield could benefit from additional validation:
- Add minimum value check (should be >= 0)
- Consider using a more specific type (e.g., milliseconds, seconds)
Example improvement:
- timeLeft: z.number(), + timeLeft: z.number().min(0).describe('Time left in milliseconds'),frontend/src/features/shared-schedule/api/queries.ts (1)
7-9: Add error handling and retry configuration.The query hooks could benefit from:
- Error type definitions for better error handling
- Retry configuration for failed requests
- Loading state handling
Example improvement for one hook:
export const useUpcomingQuery = () => useQuery<UpcomingSchedulesResponse>( sharedSchedulesQueryOptions.upcoming, + { + retry: (failureCount, error) => { + if (error instanceof NetworkError) return failureCount < 3; + return false; + }, + onError: (error) => { + // Handle error (e.g., show toast notification) + } + } );Also applies to: 11-14, 16-19
frontend/src/routes/_main/home/index.tsx (1)
21-22: Extract magic numbers into constants.The page size (6) is hard-coded. Consider:
- Moving it to a constants file
- Making it configurable based on screen size
Example improvement:
+const SCHEDULES_PER_PAGE = 6; + queryClient.prefetchQuery( - sharedSchedulesQueryOptions.ongoing(1, 6, 'ALL') + sharedSchedulesQueryOptions.ongoing(1, SCHEDULES_PER_PAGE, 'ALL') );frontend/src/features/shared-schedule/model/finishedSchedules.ts (1)
5-17: LGTM! Well-structured schemas with strong type safety.The schemas provide excellent type safety and runtime validation. Consider making the shared schema exportable for reuse in other schedule types.
Apply this diff to make the shared schema reusable:
-const SharedEventDtoSchema = z.object({ +export const SharedEventDtoSchema = z.object({ id: z.number(), startDateTime: zCoerceToDate, endDateTime: zCoerceToDate, });frontend/src/features/shared-schedule/ui/OngoingSchedules/ongoingScheduleListItem.css.ts (1)
12-12: Consider using min-height for better content flexibility.Using a fixed height might cause content overflow issues. Consider using
min-heightinstead to accommodate varying content lengths while maintaining consistent spacing.Apply this diff to improve content flexibility:
- height: '5rem', + minHeight: '5rem',frontend/src/features/shared-schedule/api/queryOptions.ts (1)
11-12: Extract time constants to avoid duplication.The stale and cache times are duplicated across all query configurations. Consider extracting these as named constants for better maintainability.
+const STALE_TIME = 4 * MINUTE_IN_MILLISECONDS; +const CACHE_TIME = 5 * MINUTE_IN_MILLISECONDS; export const sharedSchedulesQueryOptions = { upcoming: { queryKey: sharedScheduleQuerykeys.upcoming, queryFn: () => schedulesApi.getUpcomingSchedules(), - staleTime: 4 * MINUTE_IN_MILLISECONDS, - cacheTime: 5 * MINUTE_IN_MILLISECONDS, + staleTime: STALE_TIME, + cacheTime: CACHE_TIME, }, // Apply similar changes to ongoing and finished optionsAlso applies to: 17-18, 23-24
frontend/src/features/shared-schedule/ui/FinishedSchedules/index.tsx (1)
23-27: Add hover states to navigation icons.The clickable navigation icons should have hover states for better user interaction.
+const iconStyle = { + transition: 'fill 0.2s', + '&:hover': { + fill: vars.color.Ref.Netural[700], + }, +}; <ChevronLeft clickable={true} fill={vars.color.Ref.Netural[400]} + style={iconStyle} onClick={() => setCurrentYear(currentYear - 1)} />Also applies to: 31-35
frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx (1)
19-20: Enhance loading and error states.The current loading and error states are too basic. Consider using proper loading spinners and error components for better user experience.
- if (isPending) return <div>pending...</div>; - if (!data || !data.data) return <div>No data available</div>; + if (isPending) return <LoadingSpinner size="lg" />; + if (!data || !data.data) return ( + <ErrorState + message="일정을 불러올 수 없습니다" + action={<Button onClick={() => refetch()}>다시 시도</Button>} + /> + );frontend/src/components/Icon/component/ClockGraphic.tsx (1)
4-6: Consider handling style prop conflicts.The spread operator for styles could potentially override the cursor style. Consider merging styles in reverse order:
-style={{ cursor: clickable ? "pointer": "default", ...rest.style }} +style={{ ...rest.style, cursor: clickable ? "pointer": "default" }}frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (1)
22-23: Enhance loading and empty states.Consider using proper loading and empty state components for better user experience.
-if (isPending) return <div>pending...</div>; -if (!data || data.ongoingDiscussions.length === 0) return <div>no data available</div>; +if (isPending) return <LoadingSpinner />; +if (!data || data.ongoingDiscussions.length === 0) return <OngoingFallback />;frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleListItem.tsx (1)
41-44: Remove commented code.Either implement the update notification feature or remove the commented code.
- {/* {isUpdated && - <Text color={vars.color.Ref.Primary[600]} typo='b3M'> - 일정을 업데이트한 사람이 있어요! - </Text>} */}frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleList.tsx (1)
26-31: Add accessibility attributes to Flex containers.The Flex containers lack proper ARIA roles and labels for screen readers.
Add appropriate ARIA attributes:
<Flex className={containerStyle} direction='column' gap={600} justify='space-between' width='full' + role="region" + aria-label="완료된 일정 목록" > <Flex direction='column' justify='flex-start' width='full' + role="list" >Also applies to: 32-36
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
18-18: Address the TODO comment about date type conversion.The TODO comment indicates pending work on date type conversion.
Would you like me to help implement the date type conversion? I can provide a solution that aligns with the date utilities in your codebase.
frontend/src/components/Icon/component/CheckGraphic.tsx (1)
6-60: Consider optimizing the SVG for better performance.While the SVG implementation is correct, consider using an SVG optimizer to:
- Reduce path complexity
- Remove unnecessary decimal places
- Combine similar filter operations
You can use tools like SVGO to optimize the SVG code. Here's how to verify potential optimizations:
#!/bin/bash # Install and run SVGO to analyze potential optimizations npm install -g svgo svgo --test frontend/src/components/Icon/component/CheckGraphic.tsx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 211e47f and 6f1d038fc8dece8cdbc38a212a52810b64151256.
⛔ Files ignored due to path filters (2)
frontend/src/components/Icon/svg/check-graphic.svgis excluded by!**/*.svgfrontend/src/components/Icon/svg/clock-graphic.svgis excluded by!**/*.svg
📒 Files selected for processing (53)
frontend/src/components/Icon/component/CheckGraphic.tsx(1 hunks)frontend/src/components/Icon/component/ClockGraphic.tsx(1 hunks)frontend/src/components/Icon/index.ts(1 hunks)frontend/src/components/SegmentControl/ControlButton.tsx(2 hunks)frontend/src/components/SegmentControl/SegmentControl.stories.tsx(1 hunks)frontend/src/components/SegmentControl/index.tsx(3 hunks)frontend/src/features/discussion/api/queries.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionRank/index.tsx(1 hunks)frontend/src/features/shared-schedule/api/index.ts(1 hunks)frontend/src/features/shared-schedule/api/keys.ts(1 hunks)frontend/src/features/shared-schedule/api/prefetch.ts(1 hunks)frontend/src/features/shared-schedule/api/queries.ts(1 hunks)frontend/src/features/shared-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/shared-schedule/model/finishedSchedules.ts(1 hunks)frontend/src/features/shared-schedule/model/index.ts(1 hunks)frontend/src/features/shared-schedule/model/ongoingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/ui/ExpiredSchedules/ExpiredScheduleList.tsx(0 hunks)frontend/src/features/shared-schedule/ui/ExpiredSchedules/index.tsx(0 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/FinishedFallback.tsx(1 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/OngoingFallback.tsx(1 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/UpcomingFallback.tsx(1 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/index.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleList.tsx(1 hunks)frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx(3 hunks)frontend/src/features/shared-schedule/ui/FinishedSchedules/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleListItem.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/index.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ongoingScheduleListItem.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/ScheduleDetails.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleList.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleListItem.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/index.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx(2 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx(2 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/scheduleCard.css.ts(2 hunks)frontend/src/hooks/usePagination.ts(1 hunks)frontend/src/pages/HomePage/index.tsx(2 hunks)frontend/src/pages/UpcomingSchedulePage/index.css.ts(1 hunks)frontend/src/pages/UpcomingSchedulePage/index.tsx(1 hunks)frontend/src/routes/_main/discussion/candidate/index.lazy.tsx(1 hunks)frontend/src/routes/_main/home/index.lazy.tsx(0 hunks)frontend/src/routes/_main/home/index.tsx(1 hunks)frontend/src/utils/date/date.ts(2 hunks)frontend/src/utils/date/format.ts(4 hunks)frontend/src/utils/date/time.ts(1 hunks)frontend/src/utils/zod/index.ts(1 hunks)
💤 Files with no reviewable changes (7)
- frontend/src/features/shared-schedule/ui/ExpiredSchedules/index.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleListItem.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/ScheduleDetails.tsx
- frontend/src/features/shared-schedule/ui/ExpiredSchedules/ExpiredScheduleList.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleList.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/index.tsx
- frontend/src/routes/_main/home/index.lazy.tsx
✅ Files skipped from review due to trivial changes (4)
- frontend/src/features/discussion/api/queries.ts
- frontend/src/components/Icon/index.ts
- frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.css.ts
- frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (29)
frontend/src/features/shared-schedule/ui/Fallbacks/index.css.ts (1)
1-3: LGTM! Well-structured imports.Good practice using a centralized theme module for consistent styling across components.
frontend/src/features/shared-schedule/ui/UpcomingSchedules/scheduleCard.css.ts (1)
14-14: LGTM! Semantic improvement in variant naming.The change from 'selected' to 'latest' better reflects the purpose of highlighting the most recent schedule, improving code readability and maintainability.
Also applies to: 37-37
frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx (1)
13-16: LGTM! Props interface is now more data-driven.The change from a simple
selectedboolean to ascheduleobject withlatestflag improves type safety and makes the component's data requirements explicit.frontend/src/components/SegmentControl/ControlButton.tsx (1)
4-4: LGTM! Props interface refactoring improves type safety.The change from
value: stringtosegmentOption: SegmentOptionprovides better type safety and clearer separation between display text and internal values.Also applies to: 7-10
frontend/src/components/SegmentControl/SegmentControl.stories.tsx (1)
22-28: LGTM! Story updates clearly demonstrate the new API.The story updates effectively showcase the new
segmentOptionsprop structure with clear examples of label/value pairs.frontend/src/components/SegmentControl/index.tsx (1)
10-13: LGTM! Clear and well-structured SegmentOption interface.The new interface provides a clear contract for segment options with explicit types for both label and value.
frontend/src/hooks/usePagination.ts (1)
12-17: LGTM!The return object provides a clean interface with all necessary pagination controls.
frontend/src/features/shared-schedule/model/index.ts (2)
1-3: LGTM!The exports are well-organized and follow a consistent naming pattern, making the code structure clear and maintainable.
5-5: LGTM!The
MeetingMethodtype is well-defined with clear, self-documenting values.frontend/src/features/shared-schedule/api/keys.ts (1)
3-7: LGTM!The query keys are well-structured and follow React Query best practices:
- Properly typed parameters
- Consistent key structure for cache management
- Efficient cache invalidation support
frontend/src/utils/zod/index.ts (1)
11-12: LGTM!The date coercion utility is well-implemented:
- Comprehensive union type covering all common date formats
- Efficient use of Zod's pipe and coerce features
frontend/src/routes/_main/discussion/candidate/index.lazy.tsx (1)
13-15:Details
❓ Verification inconclusive
Verify the impact of route path changes.
The route path has been updated to include the '_main' prefix. Please ensure that:
- All route references are updated accordingly
- Navigation components handle the new path correctly
- No broken links exist in the application
🏁 Script executed:
#!/bin/bash # Search for potential references to the old route path rg -i "discussion/candidate" --type ts --type tsxLength of output: 77
Below is a revised review comment recommending further verification due to the error from the initial search command. Please re-run the check using the script below to ensure that all route references (both old and new) in both
.tsand.tsxfiles have been updated correctly.
Verify that all route references reflect the new '_main' prefix.
The update to include the '_main' prefix appears correct for the route definition. However, please perform these verifications before merging:
- Ensure that there are no lingering references to the old "discussion/candidate" path in the codebase.
- Confirm that all navigation components and links use the updated path "/_main/discussion/candidate/".
- Manually verify that no broken links exist after the path change.
To help with this, re-run the following script:
#!/bin/bash echo "Searching for old route references 'discussion/candidate' in ts and tsx files" rg -i "discussion/candidate" --glob "*.ts" --glob "*.tsx" echo "Searching for new route references '/_main/discussion/candidate/' in ts and tsx files" rg -i "/_main/discussion/candidate/" --glob "*.ts" --glob "*.tsx"After confirming the outputs, please update the review accordingly.
frontend/src/features/shared-schedule/ui/OngoingSchedules/index.css.ts (1)
13-15: LGTM! Style changes improve layout and spacingThe addition of full-width segment control and space-between justification enhances the visual layout and content distribution.
Also applies to: 20-20
frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1)
14-16: LGTM! Well-structured schema implementationThe schema implementation is clean and type-safe, with good use of Zod features.
frontend/src/features/shared-schedule/ui/Fallbacks/OngoingFallback.tsx (3)
14-15: Consider using responsive dimensions (same as FinishedFallback)The fixed dimensions might cause layout issues on different screen sizes. Consider using relative units or making the component more responsive.
18-18: Move text to i18n/constants file (same as FinishedFallback)Consider moving the Korean text to a dedicated i18n or constants file for better maintainability and localization support.
6-6: LGTM! Good use of shared stylesThe component properly utilizes shared styles through containerStyle import.
Also applies to: 11-11
frontend/src/features/shared-schedule/model/ongoingSchedules.ts (1)
14-20: LGTM! Well-structured pagination response schema.The pagination schema is well-defined with all necessary fields for handling paginated data.
frontend/src/pages/HomePage/index.tsx (1)
15-27: LGTM! Clean component structure.The changes align well with the PR objectives. The component structure follows a logical order: upcoming, ongoing, and finished schedules.
frontend/src/features/shared-schedule/api/queryOptions.ts (1)
7-26: Well-structured query configurations!The query options are well-organized with proper typing and separation of concerns. The stale/cache time configuration ensures efficient data freshness management.
frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
8-10: Implement the commented interface.The commented out interface suggests this component should receive a
discussionId. Consider implementing this to properly connect with API data.frontend/src/features/shared-schedule/ui/FinishedSchedules/index.tsx (1)
22-27: Implement year validation for navigation.The TODO comment indicates missing year validation. Consider adding upper/lower bounds to prevent navigating to invalid years.
+const MIN_YEAR = 2020; // Define based on business requirements +const MAX_YEAR = new Date().getFullYear(); const FinishedSchedules = () => { - const [currentYear, setCurrentYear] = useState(new Date().getFullYear()); + const [currentYear, setCurrentYear] = useState(MAX_YEAR); + + const handleYearChange = (delta: number) => { + const newYear = currentYear + delta; + if (newYear >= MIN_YEAR && newYear <= MAX_YEAR) { + setCurrentYear(newYear); + } + }; // ... - onClick={() => setCurrentYear(currentYear - 1)} + onClick={() => handleYearChange(-1)}frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx (1)
11-14: Well-implemented data fetching!The switch from prop-based to query-based data fetching is well done. The use of the nullish coalescing operator for default empty array is a good practice.
frontend/src/components/Icon/component/ClockGraphic.tsx (1)
6-23: LGTM! Well-structured SVG implementation.The SVG implementation is well-organized with proper accessibility attributes and gradient definitions.
frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx (1)
40-40:Details
❓ Verification inconclusive
Verify initial selectedDiscussionId.
The selectedDiscussionId is initialized to 1 without verifying if a discussion with that ID exists.
🏁 Script executed:
#!/bin/bash # Check if discussion with ID 1 exists in the codebase rg -A 5 "id: 1" --type tsLength of output: 6751
Address initialization of selectedDiscussionId
The current initialization of
selectedDiscussionIdto 1 assumes a discussion with this ID exists. While our codebase search shows that discussion objects withid: 1appear in various parts (e.g., story data for Checkbox, Toggle, DiscussionCalendar, and DiscussionRank), this static assignment may not reliably match the actual discussion list used in the OngoingSchedules component.
- Recommendation: Consider dynamically selecting the initial discussion ID based on the available discussions. For example, default to the first discussion’s ID when the discussion list is loaded, instead of hardcoding it to 1.
- Action: Verify that the discussion data used in OngoingSchedules indeed includes an entry with
id: 1at runtime. If not, update the initialization logic accordingly.frontend/src/components/Icon/component/CheckGraphic.tsx (1)
2-4: LGTM! Well-structured props with sensible defaults.The component follows React best practices by:
- Using a dedicated props interface
- Providing default values
- Supporting prop spreading for flexibility
frontend/src/utils/date/date.ts (3)
3-5: LGTM! Good use of a named constant for time calculations.Extracting the milliseconds-per-day value into a constant improves maintainability and reduces the risk of calculation errors.
224-238: LGTM! Clean implementation with good edge case handling.The function correctly:
- Handles same-day and different-day cases
- Uses consistent time formatting
- Provides a clear and readable output format
213-222:⚠️ Potential issueFix the logical error in year comparison.
The
isSameYearcheck is inverted, causing the wrong format to be used. The year should only be included when the years are different.Apply this fix:
- const isSameYear = startY !== endY; + const isSameYear = startY === endY;Likely an incorrect or invalid review comment.
frontend/src/hooks/usePagination.ts
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and explicit return type.
The hook should validate its input parameters and have an explicit return type for better type safety.
Apply this diff to add validation and type safety:
+interface PaginationHook {
+ currentPage: number;
+ onPageChange: (page: number) => void;
+ totalPages: number;
+}
export const usePagination = (
initialPage: number,
totalPages: number,
-) => {
+): PaginationHook => {
+ if (totalPages < 0) {
+ throw new Error('totalPages must be non-negative');
+ }
+ if (initialPage < 1 || initialPage > totalPages) {
+ throw new Error('initialPage must be between 1 and totalPages');
+ }📝 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.
| export const usePagination = ( | |
| initialPage: number, | |
| totalPages: number, | |
| ) => { | |
| interface PaginationHook { | |
| currentPage: number; | |
| onPageChange: (page: number) => void; | |
| totalPages: number; | |
| } | |
| export const usePagination = ( | |
| initialPage: number, | |
| totalPages: number, | |
| ): PaginationHook => { | |
| if (totalPages < 0) { | |
| throw new Error('totalPages must be non-negative'); | |
| } | |
| if (initialPage < 1 || initialPage > totalPages) { | |
| throw new Error('initialPage must be between 1 and totalPages'); | |
| } | |
| // ... remaining hook implementation ... | |
| } |
frontend/src/hooks/usePagination.ts
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add page validation and memoize the callback.
The page change handler should validate the new page number and be memoized to prevent unnecessary re-renders.
Apply this diff to add validation and memoization:
+import { useState, useCallback } from 'react';
- const onPageChange = (page: number) => {
+ const onPageChange = useCallback((page: number) => {
+ if (page < 1 || page > totalPages) {
+ throw new Error('Page must be between 1 and totalPages');
+ }
setCurrentPage(page);
- };
+ }, [totalPages]);📝 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.
| const onPageChange = (page: number) => { | |
| setCurrentPage(page); | |
| }; | |
| import { useState, useCallback } from 'react'; | |
| // ... other code may be present above | |
| const onPageChange = useCallback((page: number) => { | |
| if (page < 1 || page > totalPages) { | |
| throw new Error('Page must be between 1 and totalPages'); | |
| } | |
| setCurrentPage(page); | |
| }, [totalPages]); | |
| // ... rest of the code |
There was a problem hiding this comment.
Rename component to match file name.
The component is named OngoingFallback but exists in UpcomingFallback.tsx. This mismatch could lead to confusion. Consider renaming the component to UpcomingFallback to maintain consistency.
-const OngoingFallback = () => (
+const UpcomingFallback = () => (
<Flex
align='center'
className={containerStyle}
direction='column'
gap={600}
height='19.75rem'
width='83.25rem'
>
<CheckGraphic height={180} width={180} />
<Text color={vars.color.Ref.Netural[700]} typo='h3'>아직 다가오는 일정이 없어요!</Text>
</Flex>
);📝 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.
| const OngoingFallback = () => ( | |
| <Flex | |
| align='center' | |
| className={containerStyle} | |
| direction='column' | |
| gap={600} | |
| height='19.75rem' | |
| width='83.25rem' | |
| > | |
| <CheckGraphic height={180} width={180} /> | |
| <Text color={vars.color.Ref.Netural[700]} typo='h3'>아직 다가오는 일정이 없어요!</Text> | |
| </Flex> | |
| ); | |
| const UpcomingFallback = () => ( | |
| <Flex | |
| align='center' | |
| className={containerStyle} | |
| direction='column' | |
| gap={600} | |
| height='19.75rem' | |
| width='83.25rem' | |
| > | |
| <CheckGraphic height={180} width={180} /> | |
| <Text color={vars.color.Ref.Netural[700]} typo='h3'>아직 다가오는 일정이 없어요!</Text> | |
| </Flex> | |
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array index as key.
Using array indices as React keys can lead to issues with component updates and reordering. Consider using a unique identifier from the schedule object.
- key={index}
+ key={schedule.id} // Assuming schedule has an id field📝 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.
| key={index} | |
| key={schedule.id} // Assuming schedule has an id field |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary for graceful failure handling.
The Home component should be wrapped with an error boundary to handle rendering errors gracefully.
Example improvement:
+import { ErrorBoundary } from '@/components/ErrorBoundary';
+
const Home = () => (
<>
<GlobalNavBar>
<GlobalNavBar.MyCalendarLink />
<GlobalNavBar.NewDiscussionLink />
</GlobalNavBar>
+ <ErrorBoundary fallback={<HomePage.ErrorFallback />}>
<HomePage />
+ </ErrorBoundary>
</>
);📝 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.
| const Home = () => ( | |
| <> | |
| <GlobalNavBar> | |
| <GlobalNavBar.MyCalendarLink /> | |
| <GlobalNavBar.NewDiscussionLink /> | |
| </GlobalNavBar> | |
| <HomePage /> | |
| </> | |
| ); | |
| import { ErrorBoundary } from '@/components/ErrorBoundary'; | |
| const Home = () => ( | |
| <> | |
| <GlobalNavBar> | |
| <GlobalNavBar.MyCalendarLink /> | |
| <GlobalNavBar.NewDiscussionLink /> | |
| </GlobalNavBar> | |
| <ErrorBoundary fallback={<HomePage.ErrorFallback />}> | |
| <HomePage /> | |
| </ErrorBoundary> | |
| </> | |
| ); |
There was a problem hiding this comment.
Improve data existence check logic.
The current implementation only checks the first page with hardcoded parameters, which might miss data on other pages.
Consider checking total count instead:
- if (queryClient.getQueryData<OngoingSchedulesResponse>(
- sharedScheduleQuerykeys.ongoing(1, 6, 'ALL'),
- )?.totalPages === 0)
+ const data = queryClient.getQueryData<OngoingSchedulesResponse>(
+ sharedScheduleQuerykeys.ongoing(1, 1, 'ALL'),
+ );
+ if (!data || data.totalCount === 0)📝 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.
| if (queryClient.getQueryData<OngoingSchedulesResponse>( | |
| sharedScheduleQuerykeys.ongoing(1, 6, 'ALL'), | |
| )?.totalPages === 0) | |
| return <OngoingFallback />; | |
| const data = queryClient.getQueryData<OngoingSchedulesResponse>( | |
| sharedScheduleQuerykeys.ongoing(1, 1, 'ALL'), | |
| ); | |
| if (!data || data.totalCount === 0) | |
| return <OngoingFallback />; |
There was a problem hiding this comment.
Remove or implement commented code.
The commented code for date formatting should either be implemented or removed.
Consider implementing the date formatting using the existing date utilities:
- <Text color={vars.color.Ref.Red[500]} typo='b3M'>
- {/* {`마감까지 ${getDday(dateRangeEnd)}일`} */}
- </Text>
+ <Text color={vars.color.Ref.Red[500]} typo='b3M'>
+ {`마감까지 ${formatDateToDdayString(discussion.dateRangeEnd)}일`}
+ </Text>Also applies to: 66-67
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for link copy functionality.
The "링크 복사" (Copy Link) button lacks error handling for clipboard operations.
Add error handling:
- <Button size='xl' style='borderless'>링크 복사</Button>
+ <Button
+ onClick={async () => {
+ try {
+ await navigator.clipboard.writeText(window.location.href);
+ // TODO: Add toast notification for success
+ } catch (error) {
+ // TODO: Add toast notification for error
+ }
+ }}
+ size='xl'
+ style='borderless'
+ >
+ 링크 복사
+ </Button>📝 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.
| <Button size='xl' style='borderless'>링크 복사</Button> | |
| <Button | |
| onClick={async () => { | |
| try { | |
| await navigator.clipboard.writeText(window.location.href); | |
| // TODO: Add toast notification for success | |
| } catch (error) { | |
| // TODO: Add toast notification for error | |
| } | |
| }} | |
| size='xl' | |
| style='borderless' | |
| > | |
| 링크 복사 | |
| </Button> |
frontend/src/utils/date/format.ts
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and optimize the D-day calculation.
The formatDateToDdayString function lacks input validation and could be more efficient.
Consider this improved implementation:
export const formatDateToDdayString = (date: Date): string => {
+ if (!(date instanceof Date) || isNaN(date.getTime())) {
+ throw new Error('Invalid date provided');
+ }
const diffDays = getDday(date);
-
- if (diffDays === 0) return 'D-Day';
- if (diffDays > 0) return `D-${diffDays}`;
- return `D+${Math.abs(diffDays)}`;
+ return diffDays === 0 ? 'D-Day' : `D${diffDays > 0 ? '-' : '+'}${Math.abs(diffDays)}`;
};📝 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.
| export const formatDateToDdayString = (date: Date): string => { | |
| const diffDays = getDday(date); | |
| if (diffDays === 0) return 'D-Day'; | |
| if (diffDays > 0) return `D-${diffDays}`; | |
| return `D+${Math.abs(diffDays)}`; | |
| }; | |
| export const formatDateToDdayString = (date: Date): string => { | |
| if (!(date instanceof Date) || isNaN(date.getTime())) { | |
| throw new Error('Invalid date provided'); | |
| } | |
| const diffDays = getDday(date); | |
| return diffDays === 0 ? 'D-Day' : `D${diffDays > 0 ? '-' : '+'}${Math.abs(diffDays)}`; | |
| }; |
💡 Verification agent
❓ Verification inconclusive
Add unit tests for the new D-day formatting function.
The new formatDateToDdayString function requires test coverage.
Would you like me to help create unit tests for this function? I can provide test cases covering various scenarios including edge cases.
🏁 Script executed:
#!/bin/bash
# Check for existing tests
rg -g "*.test.ts" "formatDateToDdayString"Length of output: 42
Unit tests required for formatDateToDdayString
The new D-day formatting function currently lacks test coverage. Please add unit tests that cover the following scenarios:
- Zero offset: When
getDday(date)returns 0, ensure it outputs"D-Day". - Positive offset: When
getDday(date)returns a positive value, verify it outputs"D-{n}"(e.g.,"D-3"). - Negative offset: When
getDday(date)returns a negative value, verify it outputs"D+{n}"(e.g.,"D+2").
Consider placing these tests in a new file at frontend/src/utils/date/format.test.ts.
Would you like assistance creating these test cases?
frontend/src/utils/date/date.ts
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider timezone handling in D-day calculation.
The current implementation might have off-by-one errors near midnight due to timezone differences. Consider normalizing the dates to midnight in the local timezone.
Apply this improvement:
export const getDday = (date: Date): number => {
- const today = new Date();
+ const today = new Date();
+ today.setHours(0, 0, 0, 0);
+ const targetDate = new Date(date);
+ targetDate.setHours(0, 0, 0, 0);
- const diff = date.getTime() - today.getTime();
+ const diff = targetDate.getTime() - today.getTime();
return Math.floor(diff / DAY_IN_MILLISECONDS);
};📝 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.
| export const getDday = (date: Date): number => { | |
| const today = new Date(); | |
| const diff = date.getTime() - today.getTime(); | |
| return Math.floor(diff / DAY_IN_MILLISECONDS); | |
| }; | |
| export const getDday = (date: Date): number => { | |
| const today = new Date(); | |
| today.setHours(0, 0, 0, 0); | |
| const targetDate = new Date(date); | |
| targetDate.setHours(0, 0, 0, 0); | |
| const diff = targetDate.getTime() - today.getTime(); | |
| return Math.floor(diff / DAY_IN_MILLISECONDS); | |
| }; |
hamo-o
left a comment
hamo-o
left a comment
There was a problem hiding this comment.
오 저도 바꾸신대로 연도에 따라서 다르게 렌더링 하는 것이 좋을 것 같습니다. 수고하셨습니다!
There was a problem hiding this comment.
p3;
https://tanstack.com/query/latest/docs/reference/QueryClient#queryclient
요 문서 참고해서 전역적으로 설정해주시면 좋을 것 같습니다.
There was a problem hiding this comment.
반영했습니다~
d4998d2
일단 문서에서 본 대로 staletime = Infinity로 설정했는데 이래도 괜찮나 싶긴 하네요
cacheTime만 제대로 설정해 주면 별 탈 없을 것 같긴 하지만요..
There was a problem hiding this comment.
p3;
로딩화면 디자인에서 주신 애니메이션으로 넣어주면 좋을 것 같습니다.
There was a problem hiding this comment.
맞다 .. 다음 PR에서 구현해보겠습니다
There was a problem hiding this comment.
p3;
일관성을 위해서 구현하신 훅을 사용해주시면 좋을 것 같습니다! (페이지네이션을 하는 부분에서 로직 수정이 있을 때 용이할 것 같습니다.)
There was a problem hiding this comment.
p3;
페이지네이션에서, 다음 페이지의 데이터를 prefetch 해오도록 처리하면 좋을 것 같아요.
…oing schedules API types
dioo1461
force-pushed
the
feature/fe/home-page
branch
from
6f1d038 to
797402a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
frontend/src/utils/date/time.ts (1)
31-35: 🛠️ Refactor suggestionReuse the MINUTE_IN_MILLISECONDS constant.
The constant is defined twice. Reuse the exported constant to maintain DRY principle.
export const getMinuteDiff = (startTime: Date, endTime: Date): number => { - const MINUTE_IN_MILLISECONDS = 60000; const diff = endTime.getTime() - startTime.getTime(); return Math.floor(diff / MINUTE_IN_MILLISECONDS); };
♻️ Duplicate comments (3)
frontend/src/utils/date/date.ts (1)
240-244:⚠️ Potential issueConsider timezone handling in D-day calculation.
The current implementation might have off-by-one errors near midnight due to timezone differences.
Apply this improvement:
export const getDday = (date: Date): number => { - const today = new Date(); + const today = new Date(); + today.setHours(0, 0, 0, 0); + const targetDate = new Date(date); + targetDate.setHours(0, 0, 0, 0); - const diff = date.getTime() - today.getTime(); + const diff = targetDate.getTime() - today.getTime(); return Math.floor(diff / DAY_IN_MILLISECONDS); };frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (2)
15-17:⚠️ Potential issueReplace hardcoded schedule items with dynamic data.
The component currently uses multiple hardcoded instances of RecommendedScheduleItem.
Apply this diff to use dynamic data:
- <RecommendedScheduleItem /> - <RecommendedScheduleItem /> - <RecommendedScheduleItem /> + {recommendations.map((recommendation) => ( + <RecommendedScheduleItem + key={recommendation.id} + {...recommendation} + /> + ))}
29-30:⚠️ Potential issueReplace hardcoded date and time with dynamic values.
The schedule item contains hardcoded date and time values.
Apply this diff to use dynamic data:
- <Text typo='b2M'>12월 11일 목요일</Text> - <Text color={vars.color.Ref.Netural[700]} typo='b3R'>오전 11시 ~ 오후 2시 (3시간)</Text> + <Text typo='b2M'>{formatDate(date)}</Text> + <Text color={vars.color.Ref.Netural[700]} typo='b3R'> + {formatTimeRange(startTime, endTime)} + </Text>
🧹 Nitpick comments (12)
frontend/src/pages/HomePage/index.css.ts (1)
3-11: Consider using design tokens for spacing values.The spacing values (
marginTop: 80,gap: 120,paddingBottom: 126) appear to be magic numbers. Consider extracting these values into design tokens or constants for better maintainability and consistency.+// Define spacing tokens at the top of the file or in a separate tokens file +const SPACING = { + CONTAINER_TOP: 80, + SECTION_GAP: 120, + CONTAINER_BOTTOM: 126, +} as const; export const containerStyle = style({ width: 1284, - marginTop: 80, + marginTop: SPACING.CONTAINER_TOP, display: 'flex', flexDirection: 'column', - gap: 120, + gap: SPACING.SECTION_GAP, - paddingBottom: 126, + paddingBottom: SPACING.CONTAINER_BOTTOM, });frontend/src/utils/zod/index.ts (1)
16-17: Consider adding error handling for time coercion.While the implementation is correct, consider wrapping the
parseTimetransformation in a try-catch block to handle potential parsing errors gracefully.export const zCoerceToTime = z.string().time() - .transform((timeStr) => parseTime(timeStr)); + .transform((timeStr) => { + try { + return parseTime(timeStr); + } catch (error) { + throw new z.ZodError([{ + code: z.ZodIssueCode.custom, + path: [], + message: `Invalid time format: ${error.message}` + }]); + } + });frontend/src/features/shared-schedule/model/ongoingSchedules.ts (1)
14-20: Consider adding min/max constraints for pagination fields.While the schema is well-structured, consider adding constraints for pagination fields to prevent potential issues.
export const OngoingSchedulesResponseSchema = z.object({ - currentPage: z.number(), - totalPages: z.number(), + currentPage: z.number().min(1), + totalPages: z.number().min(0), hasNext: z.boolean(), hasPrevious: z.boolean(), ongoingDiscussions: z.array(OngoingScheduleSchema), });frontend/src/utils/date/time.ts (1)
63-82: Consider adding regex validation for time format.While the current validation is thorough, consider adding a regex check for the time format before splitting the string.
export const parseTime = (timeStr: string): Time => { + const TIME_FORMAT = /^([0-1]?[0-9]|2[0-3]):[0-5][0-9](:[0-5][0-9])?$/; + if (!TIME_FORMAT.test(timeStr)) { + throw new Error('parseTime: Invalid time format'); + } + const parts = timeStr.trim().split(':'); - if (parts.length < 2 || parts.length > 3) { - throw new Error('parseTime: Invalid time format'); - }frontend/src/features/discussion/model/invitation.ts (1)
5-15: Consider adding validation rules for better data integrity.While the schema structure is good, consider adding these validations:
- Title length constraints
- Host name format validation
- Reasonable duration range limits
- Ensure dateRangeEnd is after dateRangeStart
- Ensure timeRangeEnd is after timeRangeStart
Example enhancement:
export const InvitationResponseSchema = z.object({ - host: z.string(), - title: z.string(), + host: z.string().min(1).max(50), + title: z.string().min(1).max(100), dateRangeStart: zCoerceToDate, dateRangeEnd: zCoerceToDate, timeRangeStart: zCoerceToTime, timeRangeEnd: zCoerceToTime, - duration: z.number(), + duration: z.number().min(15).max(480), // 15 mins to 8 hours isFull: z.boolean(), requirePassword: z.boolean(), }).refine( + (data) => data.dateRangeEnd >= data.dateRangeStart, + { message: "End date must be after start date" } +).refine( + (data) => data.timeRangeEnd > data.timeRangeStart, + { message: "End time must be after start time" } );frontend/src/features/shared-schedule/api/prefetch.ts (1)
19-23: Consider parameterizing the hardcoded values.The function uses hardcoded values (page: 1, size: 10, current year) which reduces flexibility and reusability.
-export const prefetchFinishedSchedules = async (queryClient: QueryClient) => { +export const prefetchFinishedSchedules = async ( + queryClient: QueryClient, + page = 1, + size = 10, + year = new Date().getFullYear(), +) => { await queryClient.prefetchQuery( - sharedSchedulesQueryOptions.finished(1, 10, new Date().getFullYear()), + sharedSchedulesQueryOptions.finished(page, size, year), ); };frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx (1)
20-20: Enhance the loading state UI.The current loading state shows a simple text message. Consider using a proper loading skeleton or spinner component for better user experience.
- if (isPending) return <div>pending...</div>; + if (isPending) return <UpcomingSchedulesSkeleton />;frontend/src/components/Icon/component/CheckGraphic.tsx (3)
2-4: Remove unused props.The
strokeandfillprops are declared but not utilized in the SVG implementation. Consider removing these props to maintain a clean API surface.-export const CheckGraphic = ({ clickable = false, className, width = 24, height = 24 , stroke = "white", fill = "white", ...rest }: IconProps) => { +export const CheckGraphic = ({ clickable = false, className, width = 24, height = 24, ...rest }: IconProps) => {
6-6: Improve style handling.The current style spread could override the cursor style. Consider merging styles in reverse order to ensure cursor behavior is preserved.
-<svg width={width} height={height || width} viewBox="0 0 180 180" xmlns="http://www.w3.org/2000/svg" aria-label="check-graphic icon" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}> +<svg width={width} height={height || width} viewBox="0 0 180 180" xmlns="http://www.w3.org/2000/svg" aria-label="check-graphic icon" fill="none" className={className} style={{ ...rest.style, cursor: clickable ? "pointer": "default" }} {...rest}>
7-59: Consider namespacing SVG IDs.The SVG uses IDs like
clip0_894_193which could potentially clash with other SVGs in the application. Consider adding a component-specific prefix.Example for one ID (apply similar pattern to others):
-<g clipPath="url(#clip0_894_193)"> +<g clipPath="url(#CheckGraphic_clip0)">frontend/src/components/SegmentControl/index.tsx (1)
28-28: Consider more robust default value handling.The current default value fallback could be improved to handle edge cases better.
- defaultValue = segmentOptions[0]?.value ?? '', + defaultValue = segmentOptions?.[0]?.value ?? '',Also consider validating that the defaultValue exists in segmentOptions to prevent invalid states.
frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx (1)
52-55: Improve type safety by avoiding type assertion.The TODO comment indicates a need for generic typing. Consider implementing proper type constraints instead of using type assertion.
- onButtonHover={(value) => prefetchOngoingSchedules( - queryClient, 1, ONGOING_PAGE_SIZE, value as AttendType, - )} + onButtonHover={<T extends AttendType>(value: T) => prefetchOngoingSchedules( + queryClient, 1, ONGOING_PAGE_SIZE, value, + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6f1d038fc8dece8cdbc38a212a52810b64151256 and 797402a.
⛔ Files ignored due to path filters (2)
frontend/src/components/Icon/svg/check-graphic.svgis excluded by!**/*.svgfrontend/src/components/Icon/svg/clock-graphic.svgis excluded by!**/*.svg
📒 Files selected for processing (58)
frontend/src/components/Icon/component/CheckGraphic.tsx(1 hunks)frontend/src/components/Icon/component/ClockGraphic.tsx(1 hunks)frontend/src/components/Icon/index.ts(1 hunks)frontend/src/components/Pagination/PaginationItem.tsx(2 hunks)frontend/src/components/Pagination/index.tsx(2 hunks)frontend/src/components/SegmentControl/ControlButton.tsx(1 hunks)frontend/src/components/SegmentControl/SegmentControl.stories.tsx(1 hunks)frontend/src/components/SegmentControl/index.tsx(2 hunks)frontend/src/features/discussion/api/queries.ts(1 hunks)frontend/src/features/discussion/model/invitation.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionRank/index.tsx(2 hunks)frontend/src/features/shared-schedule/api/index.ts(1 hunks)frontend/src/features/shared-schedule/api/keys.ts(1 hunks)frontend/src/features/shared-schedule/api/prefetch.ts(1 hunks)frontend/src/features/shared-schedule/api/queries.ts(1 hunks)frontend/src/features/shared-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/shared-schedule/model/finishedSchedules.ts(1 hunks)frontend/src/features/shared-schedule/model/index.ts(1 hunks)frontend/src/features/shared-schedule/model/ongoingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/ui/ExpiredSchedules/ExpiredScheduleList.tsx(0 hunks)frontend/src/features/shared-schedule/ui/ExpiredSchedules/index.tsx(0 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/FinishedFallback.tsx(1 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/OngoingFallback.tsx(1 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/UpcomingFallback.tsx(1 hunks)frontend/src/features/shared-schedule/ui/Fallbacks/index.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleList.tsx(1 hunks)frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx(3 hunks)frontend/src/features/shared-schedule/ui/FinishedSchedules/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleListItem.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/index.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ongoingScheduleListItem.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/ScheduleDetails.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleList.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleListItem.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/index.tsx(0 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx(2 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx(2 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/scheduleCard.css.ts(2 hunks)frontend/src/hooks/usePagination.ts(1 hunks)frontend/src/main.tsx(1 hunks)frontend/src/pages/HomePage/index.css.ts(1 hunks)frontend/src/pages/HomePage/index.tsx(1 hunks)frontend/src/pages/UpcomingSchedulePage/index.css.ts(1 hunks)frontend/src/pages/UpcomingSchedulePage/index.tsx(1 hunks)frontend/src/routes/_main/discussion/candidate/index.lazy.tsx(1 hunks)frontend/src/routes/_main/home/index.lazy.tsx(0 hunks)frontend/src/routes/_main/home/index.tsx(1 hunks)frontend/src/utils/date/date.ts(2 hunks)frontend/src/utils/date/format.ts(3 hunks)frontend/src/utils/date/time.ts(2 hunks)frontend/src/utils/zod/index.ts(1 hunks)
💤 Files with no reviewable changes (7)
- frontend/src/features/shared-schedule/ui/ExpiredSchedules/index.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/index.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleListItem.tsx
- frontend/src/routes/_main/home/index.lazy.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/ScheduleDetails.tsx
- frontend/src/features/shared-schedule/ui/UnConfirmedSchedules/UnconfirmedScheduleList.tsx
- frontend/src/features/shared-schedule/ui/ExpiredSchedules/ExpiredScheduleList.tsx
🚧 Files skipped from review as they are similar to previous changes (27)
- frontend/src/features/shared-schedule/ui/Fallbacks/index.css.ts
- frontend/src/features/discussion/api/queries.ts
- frontend/src/features/shared-schedule/ui/Fallbacks/OngoingFallback.tsx
- frontend/src/components/Icon/index.ts
- frontend/src/routes/_main/home/index.tsx
- frontend/src/pages/UpcomingSchedulePage/index.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/ongoingScheduleListItem.css.ts
- frontend/src/utils/date/format.ts
- frontend/src/features/shared-schedule/ui/FinishedSchedules/index.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/scheduleCard.css.ts
- frontend/src/features/shared-schedule/api/keys.ts
- frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.css.ts
- frontend/src/features/shared-schedule/api/queryOptions.ts
- frontend/src/routes/_main/discussion/candidate/index.lazy.tsx
- frontend/src/components/Icon/component/ClockGraphic.tsx
- frontend/src/pages/UpcomingSchedulePage/index.css.ts
- frontend/src/features/shared-schedule/model/index.ts
- frontend/src/features/shared-schedule/ui/Fallbacks/FinishedFallback.tsx
- frontend/src/features/shared-schedule/api/index.ts
- frontend/src/features/shared-schedule/model/upcomingSchedules.ts
- frontend/src/features/discussion/ui/DiscussionRank/index.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleListItem.tsx
- frontend/src/features/shared-schedule/api/queries.ts
- frontend/src/features/shared-schedule/model/finishedSchedules.ts
- frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/index.css.ts
🔇 Additional comments (30)
frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx (4)
7-8: LGTM! Clean interface changes.The transition from selection-based to schedule-based props improves type safety and makes the component's requirements more explicit.
Also applies to: 10-10, 13-16
18-52: LGTM! Well-structured component with good separation of concerns.The component effectively:
- Uses data-driven rendering with the schedule prop
- Delegates date/time formatting to specialized components
- Applies conditional styling based on latest status
54-66: Consider accessibility for color contrast.While the component is well-implemented, ensure that the color combinations (especially for the 'weak' style) maintain sufficient contrast ratios for accessibility.
Let's verify the color contrast ratios:
#!/usr/bin/env python3 from colorsys import rgb_to_hls import math def luminance(r, g, b): def adjust(value): value = value / 255 return value / 12.92 if value <= 0.03928 else math.pow((value + 0.055) / 1.055, 2.4) return 0.2126 * adjust(r) + 0.7152 * adjust(g) + 0.0722 * adjust(b) def contrast_ratio(l1, l2): lighter = max(l1, l2) darker = min(l1, l2) return (lighter + 0.05) / (darker + 0.05) # Test common background/foreground combinations color_pairs = [ # Format: ((bg_r, bg_g, bg_b), (fg_r, fg_g, fg_b), "description") ((255, 255, 255), (115, 115, 115), "weak style - gray on white"), ((0, 0, 0), (255, 255, 255), "filled style - white on black") ] print("Color Contrast Analysis:") for bg, fg, desc in color_pairs: bg_luminance = luminance(*bg) fg_luminance = luminance(*fg) ratio = contrast_ratio(bg_luminance, fg_luminance) print(f"\n{desc}:") print(f"Contrast ratio: {ratio:.2f}") print(f"Meets WCAG AA for large text: {ratio >= 3}") print(f"Meets WCAG AA for small text: {ratio >= 4.5}")
68-75: LGTM! Clean and focused component.The component has a single, clear responsibility and is well-typed. The use of the utility function for date formatting promotes consistency.
frontend/src/pages/HomePage/index.css.ts (1)
3-11:Details
❓ Verification inconclusive
Verify the alignment of child components.
The removal of
alignItems: 'center'(mentioned in the AI summary) could affect the alignment of child components, particularly the year selection control and schedule components. Please verify that this change doesn't adversely affect the layout.
🏁 Script executed:
#!/bin/bash # Search for any flex alignment styles in related components that might compensate for this change rg -t ts -t tsx "alignItems|align-items" frontend/src/pages/HomePage/Length of output: 99
Action Required: Verify Visual Alignment Impact
The removal of
alignItems: 'center'may affect how child components such as the year selection control and schedule components are laid out. The initial automated search encountered an issue with the file type filter (tsx), so we couldn't conclusively verify if any compensating alignment is applied elsewhere in thefrontend/src/pages/HomePage/folder.Please run the following command or manually inspect the HomePage-related components to ensure that no alternate flex alignment styles are in place that would unintentionally alter the layout:
#!/bin/bash # Search for any flex alignment styles in the HomePage directory rg "alignItems|align-items" frontend/src/pages/HomePage/frontend/src/utils/zod/index.ts (1)
13-14: LGTM! Well-structured date coercion implementation.The union type and pipe approach for date coercion is clean and handles multiple input types effectively.
frontend/src/features/shared-schedule/model/ongoingSchedules.ts (1)
5-12: LGTM! Well-structured schedule schema.The schema is well-defined with appropriate types for all fields. Good use of
zCoerceToDatefor date fields.frontend/src/features/discussion/model/invitation.ts (3)
1-3: LGTM! Clean import structure.The imports are well-organized, using proper module aliasing and separating external from internal dependencies.
26-28: LGTM! Well-structured type exports.The type definitions are correctly inferred from the schemas and follow a consistent naming convention.
17-24:Details
❓ Verification inconclusive
Verify password attempt limit implementation.
The
failedCountin the response suggests a password attempt limit feature. Ensure this is properly implemented in the backend and that the frontend handles the limit appropriately.
🏁 Script executed:
#!/bin/bash # Search for password attempt limit handling in the codebase rg -A 5 "failedCount|attempt.*limit|max.*attempts" --type tsLength of output: 639
Password Attempt Limit Verification Required
The schema definitions themselves look correct; however, the
failedCountfield is currently only declared in the frontend’s join response schema with no corresponding handling logic found from our search. Please ensure that:
- The backend implements the necessary password attempt limit logic.
- The frontend properly interprets and handles the
failedCountfield, displaying appropriate feedback when the attempt limit is reached.frontend/src/utils/date/date.ts (2)
3-5: LGTM! Well-structured constant definition.The DAY_IN_MILLISECONDS constant is correctly defined and promotes code reusability.
224-238: LGTM! Well-implemented date-time range formatting.The function has good optimization for same-day cases and clear logic for different-day cases.
frontend/src/features/shared-schedule/ui/Fallbacks/UpcomingFallback.tsx (1)
8-20: LGTM! Clean and semantic implementation of the fallback component.The component follows best practices with:
- Semantic layout using Flex component
- Clear visual hierarchy with icon and text
- Responsive design using 'full' width
frontend/src/main.tsx (1)
11-17: Review the staleTime configuration.Setting
staleTime: Infinitymeans queries will never become stale automatically. Consider if this is appropriate for your use case:
- Pros: Reduces unnecessary refetches
- Cons: Data might become outdated without manual invalidation
Consider setting a more appropriate staleTime based on your data update frequency. For example:
const queryClient = new QueryClient({ defaultOptions: { queries: { - staleTime: Infinity, + staleTime: 5 * 60 * 1000, // 5 minutes }, }, });frontend/src/components/Icon/component/CheckGraphic.tsx (1)
65-65: LGTM!The component name and displayName are properly set following React best practices.
frontend/src/hooks/usePagination.ts (1)
3-15: Add type safety, validation, and memoization.The hook implementation needs improvements for better type safety and performance.
Apply this diff to enhance the hook:
+interface PaginationHook { + currentPage: number; + onPageChange: (page: number) => void; +} -export const usePagination = ( +export const usePagination = ( initialPage: number, + totalPages: number, -) => { +): PaginationHook => { + if (initialPage < 1 || initialPage > totalPages) { + throw new Error('initialPage must be between 1 and totalPages'); + } const [currentPage, setCurrentPage] = useState(initialPage); - const onPageChange = (page: number) => { + const onPageChange = useCallback((page: number) => { + if (page < 1 || page > totalPages) { + throw new Error('Page must be between 1 and totalPages'); + } setCurrentPage(page); - }; + }, [totalPages]); return { currentPage, onPageChange, }; };frontend/src/pages/HomePage/index.tsx (1)
17-30: LGTM! Clean component structure and proper layout implementation.The changes effectively reorganize the home page layout with proper component hierarchy and navigation.
frontend/src/components/SegmentControl/SegmentControl.stories.tsx (1)
22-28: LGTM! Well-structured stories demonstrating the new API.The stories effectively showcase the new segmentOptions prop structure with proper separation of labels and values.
Also applies to: 31-43
frontend/src/components/Pagination/PaginationItem.tsx (1)
14-14: LGTM! Well-structured hover functionality implementation.The addition of the optional hover handler is clean and type-safe, following React best practices.
Also applies to: 17-22, 39-39
frontend/src/components/SegmentControl/ControlButton.tsx (1)
4-4: LGTM! Good refactor to use structured segment options.The changes improve component flexibility by using a structured object for segment options while maintaining type safety.
Also applies to: 8-10, 13-17, 19-19, 24-24, 30-30
frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleList.tsx (2)
21-22: Enhance loading and error states with user-friendly messages.The loading and error messages should be improved with proper styling and animations as discussed in previous feedback.
25-53: LGTM! Clean implementation of paginated schedule list.The component structure is well-organized with proper data fetching, pagination, and list rendering.
frontend/src/components/SegmentControl/index.tsx (1)
10-13: LGTM! Well-structured interface changes.The introduction of the SegmentOption interface and corresponding prop updates improve type safety and component flexibility.
Also applies to: 15-15, 21-21
frontend/src/components/Pagination/index.tsx (1)
16-16: LGTM! Good performance optimization.The addition of
onPageButtonHoverprop enables prefetching on hover, which can improve the user experience by preloading data before page changes.Also applies to: 23-23, 35-35
frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (2)
41-47: Replace array index with unique identifier as key prop.Using array index as key prop can lead to rendering issues when the list changes.
53-55: LGTM! Efficient data prefetching implementation.The implementation of prefetching data on pagination hover is a good optimization that will improve the user experience.
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (2)
57-58: Remove or implement commented code.The commented code for date formatting should either be implemented or removed.
Also applies to: 66-67
38-38: Add error handling for link copy functionality.The "링크 복사" (Copy Link) button lacks error handling for clipboard operations.
frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx (2)
43-46: Improve data existence check logic.The current implementation only checks the first page with hardcoded parameters, which might miss data on other pages.
28-72: LGTM! Well-structured component with good separation of concerns.The component effectively manages state and renders appropriate content based on data availability. The use of SegmentControl for filtering and the separation into Content component improves maintainability.
| export const getDateRangeString = (startDate: Date, endDate: Date): string => { | ||
| const { year: startY, month: startM, day: startD } = getDateParts(startDate); | ||
| const { year: endY, month: endM, day: endD } = getDateParts(endDate); | ||
|
|
||
| const isSameYear = startY !== endY; | ||
| const format = (year: number, month: number, day: number): string => | ||
| isSameYear ? `${year}년 ${month + 1}월 ${day}일` : `${month + 1}월 ${day}일`; | ||
|
|
||
| return `${format(startY, startM, startD)} ~ ${format(endY, endM, endD)}`; | ||
| }; |
There was a problem hiding this comment.
Fix incorrect year comparison logic.
There's a bug in the year comparison that will cause the year to always be displayed:
isSameYear = startY !== endYis the opposite of what it should be- The format function uses this incorrect condition
Apply this fix:
- const isSameYear = startY !== endY;
+ const isSameYear = startY === endY;
const format = (year: number, month: number, day: number): string =>
- isSameYear ? `${year}년 ${month + 1}월 ${day}일` : `${month + 1}월 ${day}일`;
+ isSameYear ? `${month + 1}월 ${day}일` : `${year}년 ${month + 1}월 ${day}일`;📝 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.
| export const getDateRangeString = (startDate: Date, endDate: Date): string => { | |
| const { year: startY, month: startM, day: startD } = getDateParts(startDate); | |
| const { year: endY, month: endM, day: endD } = getDateParts(endDate); | |
| const isSameYear = startY !== endY; | |
| const format = (year: number, month: number, day: number): string => | |
| isSameYear ? `${year}년 ${month + 1}월 ${day}일` : `${month + 1}월 ${day}일`; | |
| return `${format(startY, startM, startD)} ~ ${format(endY, endM, endD)}`; | |
| }; | |
| export const getDateRangeString = (startDate: Date, endDate: Date): string => { | |
| const { year: startY, month: startM, day: startD } = getDateParts(startDate); | |
| const { year: endY, month: endM, day: endD } = getDateParts(endDate); | |
| const isSameYear = startY === endY; | |
| const format = (year: number, month: number, day: number): string => | |
| isSameYear ? `${month + 1}월 ${day}일` : `${year}년 ${month + 1}월 ${day}일`; | |
| return `${format(startY, startM, startD)} ~ ${format(endY, endM, endD)}`; | |
| }; |
| export const prefetchUpcomingSchedules = async (queryClient: QueryClient) => { | ||
| await queryClient.prefetchQuery(sharedSchedulesQueryOptions.upcoming); | ||
| }; | ||
|
|
||
| export const prefetchOngoingSchedules = async ( | ||
| queryClient: QueryClient, | ||
| page: number, | ||
| size: number, | ||
| attendType: AttendType, | ||
| ) => { | ||
| await queryClient.prefetchQuery(sharedSchedulesQueryOptions.ongoing(page, size, attendType)); | ||
| }; | ||
|
|
||
| export const prefetchFinishedSchedules = async (queryClient: QueryClient) => { | ||
| await queryClient.prefetchQuery( | ||
| sharedSchedulesQueryOptions.finished(1, 10, new Date().getFullYear()), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for prefetch operations.
Consider wrapping the prefetch operations in try-catch blocks to handle potential network failures gracefully.
export const prefetchUpcomingSchedules = async (queryClient: QueryClient) => {
+ try {
await queryClient.prefetchQuery(sharedSchedulesQueryOptions.upcoming);
+ } catch (error) {
+ console.error('Failed to prefetch upcoming schedules:', error);
+ throw error;
+ }
};
export const prefetchOngoingSchedules = async (
queryClient: QueryClient,
page: number,
size: number,
attendType: AttendType,
) => {
+ try {
await queryClient.prefetchQuery(sharedSchedulesQueryOptions.ongoing(page, size, attendType));
+ } catch (error) {
+ console.error('Failed to prefetch ongoing schedules:', error);
+ throw error;
+ }
};
export const prefetchFinishedSchedules = async (queryClient: QueryClient) => {
+ try {
await queryClient.prefetchQuery(
sharedSchedulesQueryOptions.finished(1, 10, new Date().getFullYear()),
);
+ } catch (error) {
+ console.error('Failed to prefetch finished schedules:', error);
+ throw error;
+ }
};📝 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.
| export const prefetchUpcomingSchedules = async (queryClient: QueryClient) => { | |
| await queryClient.prefetchQuery(sharedSchedulesQueryOptions.upcoming); | |
| }; | |
| export const prefetchOngoingSchedules = async ( | |
| queryClient: QueryClient, | |
| page: number, | |
| size: number, | |
| attendType: AttendType, | |
| ) => { | |
| await queryClient.prefetchQuery(sharedSchedulesQueryOptions.ongoing(page, size, attendType)); | |
| }; | |
| export const prefetchFinishedSchedules = async (queryClient: QueryClient) => { | |
| await queryClient.prefetchQuery( | |
| sharedSchedulesQueryOptions.finished(1, 10, new Date().getFullYear()), | |
| ); | |
| }; | |
| export const prefetchUpcomingSchedules = async (queryClient: QueryClient) => { | |
| try { | |
| await queryClient.prefetchQuery(sharedSchedulesQueryOptions.upcoming); | |
| } catch (error) { | |
| console.error('Failed to prefetch upcoming schedules:', error); | |
| throw error; | |
| } | |
| }; | |
| export const prefetchOngoingSchedules = async ( | |
| queryClient: QueryClient, | |
| page: number, | |
| size: number, | |
| attendType: AttendType, | |
| ) => { | |
| try { | |
| await queryClient.prefetchQuery(sharedSchedulesQueryOptions.ongoing(page, size, attendType)); | |
| } catch (error) { | |
| console.error('Failed to prefetch ongoing schedules:', error); | |
| throw error; | |
| } | |
| }; | |
| export const prefetchFinishedSchedules = async (queryClient: QueryClient) => { | |
| try { | |
| await queryClient.prefetchQuery( | |
| sharedSchedulesQueryOptions.finished(1, 10, new Date().getFullYear()), | |
| ); | |
| } catch (error) { | |
| console.error('Failed to prefetch finished schedules:', error); | |
| throw error; | |
| } | |
| }; |
| const { data, isPending } = useUpcomingQuery(); | ||
| const schedules = data?.data ?? []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the query.
The query's error state is not handled. Consider adding error handling to show an appropriate error message to users.
- const { data, isPending } = useUpcomingQuery();
+ const { data, isPending, error } = useUpcomingQuery();
const schedules = data?.data ?? [];
+
+ if (error) {
+ return <ErrorMessage message="Failed to load upcoming schedules" />;
+ }📝 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.
| const { data, isPending } = useUpcomingQuery(); | |
| const schedules = data?.data ?? []; | |
| const { data, isPending, error } = useUpcomingQuery(); | |
| const schedules = data?.data ?? []; | |
| if (error) { | |
| return <ErrorMessage message="Failed to load upcoming schedules" />; | |
| } |
| // interface RecommendedSchedulesProps { | ||
| // discussionId: number; | ||
| // } |
There was a problem hiding this comment.
Uncomment and implement the interface.
The commented interface suggests this component should receive a discussionId prop. This should be implemented to fetch the correct data.
Apply this diff to enable the interface:
-// interface RecommendedSchedulesProps {
-// discussionId: number;
-// }
+interface RecommendedSchedulesProps {
+ discussionId: number;
+}
-const RecommendedSchedules = () => (
+const RecommendedSchedules = ({ discussionId }: RecommendedSchedulesProps) => (📝 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.
| // interface RecommendedSchedulesProps { | |
| // discussionId: number; | |
| // } | |
| interface RecommendedSchedulesProps { | |
| discussionId: number; | |
| } | |
| const RecommendedSchedules = ({ discussionId }: RecommendedSchedulesProps) => ( | |
| // ... rest of the component implementation | |
| ); |
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
Fallback컴포넌트를 표시합니다.확정되지 않은 일정
지난 일정
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
segment control 컴포넌트에
value만을 넘기던 기존 구현에서을 넘기도록 구현이 변경되었습니다.
button에 표시될 문구는
label에, 실제로 탭 전환을 관리할 키 값은value에 할당하면 됩니다.Summary by CodeRabbit
New Features
Improvements