[FE-Feat] 후보 일정 디테일 화면 API 연결 #228
Conversation
WalkthroughThis pull request introduces a new candidate schedule API module with corresponding query keys, a custom hook, and query options. It updates data schemas and type definitions using Zod, revises several UI components to integrate real data fetching, and adjusts component props and styling for candidate schedule, discussion, and shared schedule features. Additionally, it restructures routing and page components to pass candidate state, adds a new date formatting utility, and removes obsolete routes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CS as CandidateSchedulePage
participant Q as useCandidateDetailQuery
participant QO as candidateDetailQueryOption
participant API as candidateDetailApi
participant BE as Backend API
U->>CS: Navigate to candidate schedule
CS->>Q: Invoke useCandidateDetailQuery(discussionId, startDateTime, endDateTime, selectedUserIdList)
Q->>QO: Configure query options
QO->>API: Call getCandidateScheduleDetail(discussionId, startDateTime, endDateTime, selectedUserIdList)
API->>BE: POST /api/v1/discussion/{discussionId}/candidate-event/details
BE-->>API: Return candidate schedule details
API-->>QO: Return validated data
QO-->>Q: Deliver query response
Q-->>CS: Render candidate schedule details
Suggested labels
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: 6
🔭 Outside diff range comments (3)
frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx (1)
25-31: 🛠️ Refactor suggestionAvoid using array index as key prop.
Using array index as key prop can lead to rendering issues if the array items are reordered or modified.
Consider using a unique identifier:
{participant.events.map((event, index) => ( <TimelineBlock event={event} gridStart={gridStart} - key={index} + key={event.id} /> ))}frontend/src/features/candidate-schedule/ui/Header.tsx (2)
42-46:⚠️ Potential issueUpdate HeaderTextInfo prop types to match new naming.
The HeaderTextInfo component's props should be updated to match the new naming convention.
Apply this diff:
-const HeaderTextInfo = ({ adjustCount, startTime, endTime }: { +const HeaderTextInfo = ({ adjustCount, startDateTime, endDateTime }: { adjustCount: number; - startTime: Date; - endTime: Date; + startDateTime: Date; + endDateTime: Date;
60-60:⚠️ Potential issueUpdate formatTimeToColonString usage to match new prop names.
Update the variable names in the template literal to match the new prop names.
Apply this diff:
- {`${formatTimeToColonString(startTime)} ~ ${formatTimeToColonString(endTime)}`} + {`${formatTimeToColonString(startDateTime)} ~ ${formatTimeToColonString(endDateTime)}`}
🧹 Nitpick comments (17)
frontend/src/features/discussion/ui/DiscussionTitle/index.tsx (1)
16-16: Add type safety for discussionId conversion.The inline conversion of
params.idto number could fail if the ID is not a valid number. Consider adding validation or using a safer conversion method.- <DiscussionContext.Provider value={{ discussionId: Number(params.id) }}> + <DiscussionContext.Provider value={{ discussionId: parseInt(params.id, 10) || 0 }}>frontend/src/features/discussion/ui/DiscussionCard/index.tsx (1)
23-24: Consider safer date handling.Direct
new Date()conversion could throw for invalid dates. Consider using a date utility library or adding validation.- startDateTime: new Date(discussion.startDateTime), - endDateTime: new Date(discussion.endDateTime), + startDateTime: discussion.startDateTime ? new Date(discussion.startDateTime) : null, + endDateTime: discussion.endDateTime ? new Date(discussion.endDateTime) : null,frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx (4)
9-11: Consider extracting props to a separate interface.While the inline prop types are valid, extracting them to a separate interface would improve reusability and documentation.
+interface CalendarDateProps { + date: Date; + groupMap: Map<string, DiscussionDTO[]>; +} + -export const CalendarDate = ( - { date, groupMap }: { date: Date; groupMap: Map<string, DiscussionDTO[]> }, -) => { +export const CalendarDate = ({ date, groupMap }: CalendarDateProps) => {
14-22: Remove unnecessary key prop from Flex component.The
keyprop on the Flex component is unnecessary as it's not being rendered in a list.<Flex className={dayStyle} direction='column' gap={400} height='100%' justify='flex-start' - key={date.getTime()} width='calc(100% / 7)' >
23-29: Improve key uniqueness and add empty state handling.
- The current key combination might not be unique if multiple discussions start at the same time. Consider using a more unique identifier.
- Consider adding an empty state when there are no discussions for the day.
- {groupMap.get(day)?.map((discussion, idx) => + {groupMap.get(day)?.length ? ( + groupMap.get(day)?.map((discussion) => ( <DiscussionCard discussion={discussion} - key={`${discussion.startDateTime}-${idx}`} + key={discussion.id} // Assuming discussion has a unique id size='sm' - />, - )} + /> + )) + ) : ( + <Flex justify="center" align="center" height="100%"> + <span>No discussions</span> + </Flex> + )}
9-32: Consider adding error boundary and loading state.The component could benefit from error handling and loading state management to improve user experience.
+import { ErrorBoundary } from '@/components/ErrorBoundary'; +import { Spinner } from '@/components/Spinner'; export const CalendarDate = ({ date, groupMap }: CalendarDateProps) => { const day = WEEK[date.getDay()]; + + if (!groupMap) { + return ( + <Flex className={dayStyle} justify="center" align="center"> + <Spinner size="sm" /> + </Flex> + ); + } + return ( + <ErrorBoundary fallback={<div>Error loading discussions</div>}> <Flex className={dayStyle} direction='column' gap={400} height='100%' justify='flex-start' width='calc(100% / 7)' > {/* ... existing code ... */} </Flex> + </ErrorBoundary> ); };frontend/src/features/candidate-schedule/ui/index.tsx (4)
21-28: Add error handling for network failures.While
isPendingand the null/undefined check cover loading and empty states, an exception thrown from the query (e.g., network error) may not be handled. Consider adding a fallback or error state for improved UX.const { data, isPending, error } = useCandidateDetailQuery( discussionId, startDateTime, endDateTime, selectedParticipantIds, ); -if (isPending) return <div>Pending...</div>; -if (!data) return <div>data is undefined or null</div>; +if (error) return <div>An error occurred while fetching data</div>; +if (isPending) return <div>Pending...</div>; +if (!data) return <div>No data found</div>;
38-41: Avoid excessive spreading of large data objects.Spreading
{...data}intoContentcan obscure which properties are actually consumed and potentially introduce unintentional props. Consider being explicit about essential props.-<Content adjustCount={adjustCount} {...data} /> +<Content + adjustCount={adjustCount} + participants={data.participants} + // ...add specific fields from data here instead of spreading +/>
67-72: Validate destructured properties.Destructuring is convenient, but confirm that all required fields in
props(if any) are indeed present. A fallback or default could protect against undefined data.
79-82: Consider more descriptive naming for conflict timestamps.Renaming
conflictStart/conflictEndto something likeconflictStartTime/conflictEndTimecan make it clearer these are Date objects. This is a minor naming improvement.frontend/src/features/candidate-schedule/api/queryOptions.ts (1)
16-16: Consider increasing the cache time.The current 1-minute cache time might lead to unnecessary API calls for relatively static candidate schedule data. Consider increasing it based on how frequently the data changes.
frontend/src/features/candidate-schedule/api/index.ts (1)
5-5: Consider API organization strategy.The TODO comment raises a valid architectural question. Consider these factors for API organization:
- Cohesion: Group related endpoints together
- Maintainability: Separate concerns for easier updates
- Reusability: Avoid duplication of shared logic
Would you like to discuss the trade-offs between combining vs. separating these API modules?
frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts (1)
87-87: Remove commented out code.Remove the commented out maxWidth property as it's not being used.
Apply this diff:
- // maxWidth: '100%',frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
29-29: Improve key prop implementation.Using
JSON.stringifyin the key prop could impact performance. Consider using a more specific unique identifier.Apply this diff:
- key={`${JSON.stringify(candidate)}-${idx}`} + key={`${candidate.startDateTime}-${candidate.endDateTime}-${idx}`}frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (2)
19-19: Consider adding a ticket to track the TODO item.The TODO comment about date type conversion should be tracked in your issue management system to ensure it's not forgotten.
Would you like me to help create an issue to track this TODO item about date type conversion?
28-29: Enhance error handling with more descriptive messages.The current error messages are too generic. Consider providing more context to help users understand what went wrong.
- if (isDiscussionLoading || isCandidateLoading) return <div>pending ...</div>; - if (!discussion || !candidates) return <div>No data available</div>; + if (isDiscussionLoading || isCandidateLoading) return <div>일정 정보를 불러오는 중...</div>; + if (!discussion || !candidates) return <div>일정 정보를 찾을 수 없습니다.</div>;frontend/src/utils/date/format.ts (1)
105-133: Add JSDoc documentation and optimize string handling.The function implementation is good but lacks documentation and has some minor optimization opportunities.
Consider these improvements:
+/** + * Formats a date into Korean locale string with customizable parts. + * @param date - The date to format + * @param exclude - Options to exclude specific parts of the date + * @returns Formatted date string in Korean (e.g., "23년 2월 15일 수요일") + */ export const formatKoreanDate = ( date: Date, exclude?: { year?: boolean; month?: boolean; date?: boolean; dow?: boolean; }, ): string => { const parts = []; if (!exclude?.year) { - parts.push(date.getFullYear().toString() - .slice(2) + '년'); + parts.push(`${date.getFullYear().toString().slice(2)}년`); } if (!exclude?.month) { - parts.push((date.getMonth() + 1) + '월'); + parts.push(`${date.getMonth() + 1}월`); } if (!exclude?.date) { - parts.push(date.getDate() + '일'); + parts.push(`${date.getDate()}일`); } let result = parts.join(' '); if (!exclude?.dow) { - result += ` ${date.toLocaleString('ko-KR', { weekday: 'short' })}요일`; + const weekday = date.toLocaleString('ko-KR', { weekday: 'short' }); + result = `${result} ${weekday}요일`; } return result; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d397838 and a217af2dbbc3980759ab0bf84df4918d64822132.
📒 Files selected for processing (22)
frontend/src/features/candidate-schedule/api/index.ts(1 hunks)frontend/src/features/candidate-schedule/api/keys.ts(1 hunks)frontend/src/features/candidate-schedule/api/queries.ts(1 hunks)frontend/src/features/candidate-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/candidate-schedule/model/index.ts(1 hunks)frontend/src/features/candidate-schedule/ui/Header.tsx(2 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx(2 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts(4 hunks)frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/index.tsx(2 hunks)frontend/src/features/discussion/model/index.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCard/index.tsx(2 hunks)frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionTitle/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(3 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts(1 hunks)frontend/src/main.tsx(1 hunks)frontend/src/pages/CandidateSchedulePage/index.tsx(1 hunks)frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx(1 hunks)frontend/src/utils/date/format.ts(1 hunks)
🔇 Additional comments (22)
frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts (1)
1-10: LGTM! Well-structured context implementation.The context implementation follows React best practices with proper typing, safe context usage, and clear exports.
frontend/src/features/discussion/ui/DiscussionTitle/index.tsx (1)
17-35: LGTM! Clean implementation of the discussion title component.The component structure is well-organized with proper loading state handling and clear separation of concerns.
frontend/src/features/discussion/ui/DiscussionCard/index.tsx (2)
25-26: Address the TODO comment regarding selectedParticipantIds.The current implementation assumes all participants are selected, which may not be the intended behavior.
Could you clarify how the participant selection should be handled? I can help implement the proper selection logic once the requirements are clear.
16-36: LGTM! Clean implementation of the discussion card component.The component effectively handles navigation and state passing while maintaining a clean conditional rendering structure.
frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx (1)
1-7: LGTM! Well-organized imports.The imports are properly organized, following a clear pattern of external dependencies followed by internal modules.
frontend/src/features/candidate-schedule/ui/index.tsx (4)
5-6: Well-organized imports.These imports properly bring in the query hook and model types needed for data fetching and type checking, respectively. Good job ensuring you’re referencing the appropriate paths.
13-19: Confirm received date type alignment.You're declaring
startDateTimeandendDateTimeasDate. Verify that these props (especially if coming from an API) are indeed JavaScript Date objects rather than raw strings, which could break type assumptions in your UI.
30-31: Verify participant splitting logic.Ensure that
data.participantsis always an array. If the API returns an empty or malformed list, double-check that yoursplitParticipantsBySelectionfunction handles edge cases gracefully.
61-62: Check for property collisions.
ContentPropsextendsCandidateDetailResponsewhile addingadjustCount. Confirm there’s no naming conflict with properties inCandidateDetailResponse.frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx (1)
13-16: Dynamic route successfully defined.Switching to a dynamic route allows for flexible access to specific candidates. Ensure that all child components handle missing or invalid
$idparameters gracefully.frontend/src/features/candidate-schedule/api/queries.ts (1)
6-13: LGTM! Well-structured custom hook implementation.The hook follows React Query best practices, has proper type definitions, and maintains a clear, focused responsibility.
frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts (1)
21-24: LGTM! Clean flexbox implementation.The added flexbox properties create a well-structured horizontal layout with proper spacing and alignment.
frontend/src/features/candidate-schedule/api/index.ts (1)
7-25: LGTM! Well-structured API implementation.The implementation follows best practices:
- Strong typing for parameters
- Proper request body structure
- Response validation using Zod schema
- Clear error handling through schema parsing
frontend/src/main.tsx (2)
11-17: Review QueryClient staleTime configuration.Setting
staleTime: Infinitymeans the data will never be considered stale. This might lead to outdated data being displayed if the schedule data changes on the server.Consider adjusting the staleTime based on how frequently the schedule data changes:
const queryClient = new QueryClient({ defaultOptions: { queries: { - staleTime: Infinity, + staleTime: 5 * 60 * 1000, // 5 minutes }, }, });
31-38: LGTM! Well-structured routing state.The HistoryState interface is well-defined with proper types that align with the API parameters.
frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx (1)
15-19: LGTM! Improved flex container alignment.The addition of
justify='flex-start'ensures consistent alignment of timeline blocks.frontend/src/features/candidate-schedule/model/index.ts (2)
28-32: LGTM! Improved date handling with coercion.The use of
zCoerceToDateinstead ofz.date()improves robustness by handling date string inputs automatically.
5-9: LGTM! Better semantic naming for status.The change from 'notInRange' to 'outOfRange' improves clarity and better describes the status meaning.
frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx (1)
52-53: LGTM! Improved status terminology.The change from 'notInRange' to 'outOfRange' improves clarity and maintains consistency across the codebase.
Also applies to: 58-69
frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts (1)
11-11: LGTM! Added consistent minimum heights.The addition of minHeight ensures consistent UI rendering across different states.
Also applies to: 48-48
frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
66-75: LGTM! Well-implemented AvailableChip component.The AvailableChip component is well-structured with clear conditional rendering based on adjustCount.
frontend/src/features/discussion/model/index.ts (1)
49-54: LGTM! Good improvement in API flexibility.Making these fields optional is a good practice as it allows for more flexible API usage while maintaining type safety through Zod schema validation.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider serializing Date objects for stable query keys.
Passing raw Date objects in React Query keys can lead to unintentional cache misses if the Date instance differs. Converting them to timestamps or ISO strings helps ensure key stability. Also consider sorting or serializing the array of IDs to avoid key collisions.
-export const candidateDetailQueryKey = (
- discussionId: number,
- startDateTime: Date,
- endDateTime: Date,
- selectedUserIdList: number[],
-) => ['candidateDetail', discussionId, startDateTime, endDateTime, selectedUserIdList];
+export const candidateDetailQueryKey = (
+ discussionId: number,
+ startDateTime: Date,
+ endDateTime: Date,
+ selectedUserIdList: number[],
+) => [
+ 'candidateDetail',
+ discussionId,
+ startDateTime.toISOString(),
+ endDateTime.toISOString(),
+ [...selectedUserIdList].sort(),
+];📝 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 candidateDetailQueryKey = ( | |
| discussionId: number, | |
| startDateTime: Date, | |
| endDateTime: Date, | |
| selectedUserIdList: number[], | |
| ) => ['candidateDetail', discussionId, startDateTime, endDateTime, selectedUserIdList]; | |
| export const candidateDetailQueryKey = ( | |
| discussionId: number, | |
| startDateTime: Date, | |
| endDateTime: Date, | |
| selectedUserIdList: number[], | |
| ) => [ | |
| 'candidateDetail', | |
| discussionId, | |
| startDateTime.toISOString(), | |
| endDateTime.toISOString(), | |
| [...selectedUserIdList].sort(), | |
| ]; |
There was a problem hiding this comment.
Implement error handling for missing candidate data.
The TODO comment indicates missing error handling. This should be addressed to prevent potential runtime errors.
Consider implementing proper error boundaries or fallback UI components to handle cases where candidate data is missing.
There was a problem hiding this comment.
Remove non-null assertion operator.
Using the non-null assertion operator (!) is unsafe and could lead to runtime errors.
-<CandidateScheduleDetail discussionId={Number(id)} {...candidate!} />
+<CandidateScheduleDetail discussionId={Number(id)} {...candidate} />📝 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.
| <CandidateScheduleDetail discussionId={Number(id)} {...candidate!} /> | |
| <CandidateScheduleDetail discussionId={Number(id)} {...candidate} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type guard for candidate state.
The current implementation might lead to undefined state. Consider adding a type guard:
-const { candidate } = state ?? {};
+const { candidate } = state ?? { candidate: undefined };
+if (!candidate) {
+ return <div>No candidate data available</div>;
+}📝 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 { candidate } = state ?? {}; | |
| const { candidate } = state ?? { candidate: undefined }; | |
| if (!candidate) { | |
| return <div>No candidate data available</div>; | |
| } |
There was a problem hiding this comment.
Fix inconsistent prop names in HeaderTextInfo usage.
The props have been renamed to startDateTime and endDateTime, but they are passed as startTime and endTime to HeaderTextInfo.
Apply this diff to maintain consistency:
<HeaderTextInfo
adjustCount={adjustCount}
- endTime={endDateTime}
- startTime={startDateTime}
+ endDateTime={endDateTime}
+ startDateTime={startDateTime}
/>📝 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.
| <HeaderTextInfo | |
| adjustCount={adjustCount} | |
| endTime={endTime} | |
| startTime={startTime} | |
| endTime={endDateTime} | |
| startTime={startDateTime} | |
| /> | |
| <HeaderTextInfo | |
| adjustCount={adjustCount} | |
| endDateTime={endDateTime} | |
| startDateTime={startDateTime} | |
| /> |
There was a problem hiding this comment.
Remove non-null assertion and handle undefined case properly.
The TODO comment indicates that undefined case handling is needed, but the code uses a non-null assertion (!) which bypasses TypeScript's null checks. This is inconsistent with the TODO and could lead to runtime errors.
Consider this safer approach:
- {/* TODO: candidate 가 undefined일 경우의 예외 처리 */}
- <RecommendedSchedules
- candidates={candidates!}
- discussion={discussion}
- />
+ {candidates && (
+ <RecommendedSchedules
+ candidates={candidates}
+ discussion={discussion}
+ />
+ )}📝 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.
| {/* TODO: candidate 가 undefined일 경우의 예외 처리 */} | |
| <RecommendedSchedules | |
| candidates={candidates!} | |
| discussion={discussion} | |
| /> | |
| {candidates && ( | |
| <RecommendedSchedules | |
| candidates={candidates} | |
| discussion={discussion} | |
| /> | |
| )} |
dioo1461
force-pushed
the
feature/fe/candidate-detail-api
branch
from
a217af2 to
5cc20e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
frontend/src/features/candidate-schedule/ui/Header.tsx (1)
50-54:⚠️ Potential issueUpdate prop types to match the new naming.
The prop types in HeaderTextInfo still use the old naming convention.
Apply this diff:
-const HeaderTextInfo = ({ adjustCount, startTime, endTime }: { +const HeaderTextInfo = ({ adjustCount, startDateTime, endDateTime }: { adjustCount: number; - startTime: Date; - endTime: Date; + startDateTime: Date; + endDateTime: Date;
♻️ Duplicate comments (4)
frontend/src/pages/CandidateSchedulePage/index.tsx (2)
10-11:⚠️ Potential issuePrevious review comment is still valid.
The current implementation might lead to undefined state and the TODO comment indicates missing error handling.
Apply this diff to add proper error handling:
-const { candidate } = state ?? {}; -// TODO: candidate가 없을 경우에 대한 예외 처리 +const { candidate } = state ?? { candidate: undefined }; +if (!candidate) { + return <div>No candidate data available</div>; +}
15-15:⚠️ Potential issuePrevious review comment is still valid.
Using the non-null assertion operator (
!) is unsafe.Apply this diff to remove the non-null assertion:
-<CandidateScheduleDetail discussionId={Number(id)} {...candidate!} /> +<CandidateScheduleDetail discussionId={Number(id)} {...candidate} />frontend/src/features/candidate-schedule/ui/Header.tsx (2)
22-23:⚠️ Potential issuePrevious review comment is still valid.
The props have been renamed to
startDateTimeandendDateTime, but they are passed asstartTimeandendTimeto HeaderTextInfo.Apply this diff to maintain consistency:
<HeaderTextInfo adjustCount={adjustCount} - endTime={endDateTime} - startTime={startDateTime} + endDateTime={endDateTime} + startDateTime={startDateTime} />
70-70:⚠️ Potential issueUpdate variable names to match the new prop names.
The variable names in the template literal don't match the new prop names.
Apply this diff:
- {`${formatTimeToColonString(startTime)} ~ ${formatTimeToColonString(endTime)}`} + {`${formatTimeToColonString(startDateTime)} ~ ${formatTimeToColonString(endDateTime)}`}
🧹 Nitpick comments (8)
frontend/src/features/candidate-schedule/api/queryOptions.ts (2)
6-11: Consider using more specific types for date parameters.Instead of using
stringtype forstartDateTimeandendDateTime, consider using a more specific type likeISO8601DateTimeorDateto ensure type safety and prevent invalid date formats.Example type definition:
type ISO8601DateTime = string; export const candidateDetailQueryOption = ( discussionId: number, startDateTime: ISO8601DateTime, endDateTime: ISO8601DateTime, selectedUserIdList: number[], ) => // ...
12-17: Review cache time configuration and add error handling.The current implementation has a few potential improvements:
- The 1-minute cache time might be too short if the schedule data doesn't change frequently
- Missing error handling and retry configurations which are important for API calls
Consider adding these configurations:
export const candidateDetailQueryOption = ( // ... parameters ... ) => ({ queryKey: candidateDetailQueryKey(discussionId, startDateTime, endDateTime, selectedUserIdList), queryFn: () => candidateDetailApi.getCandidateScheduleDetail( discussionId, startDateTime, endDateTime, selectedUserIdList, ), cacheTime: 5 * MINUTE_IN_MILLISECONDS, // Increased cache time retry: 3, // Add retry attempts retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), // Exponential backoff staleTime: 1 * MINUTE_IN_MILLISECONDS, // Add stale time configuration });frontend/src/features/discussion/ui/DiscussionCard/index.tsx (1)
25-26: Address the TODO comment regarding participant selection.The current implementation assumes all users are selected by default, which may not be the desired behavior. This needs to be revisited to allow for proper participant selection.
Would you like me to help implement the participant selection logic or create an issue to track this task?
frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
69-78: Enhance maintainability of availability indicators.Consider extracting magic numbers and using more semantic color names for better maintainability.
+const ADJUSTMENT_THRESHOLD = 0; +const COLORS = { + NEEDS_ADJUSTMENT: 'red', + ALL_AVAILABLE: 'blue', +} as const; const AvailableChip = ({ adjustCount }: { adjustCount: number }) => ( <Chip - color={adjustCount > 0 ? 'red' : 'blue'} + color={adjustCount > ADJUSTMENT_THRESHOLD ? COLORS.NEEDS_ADJUSTMENT : COLORS.ALL_AVAILABLE} radius='max' size='md' style='weak' > - {adjustCount > 0 ? `조율 필요 ${adjustCount}` : '모두 가능'} + {adjustCount > ADJUSTMENT_THRESHOLD ? `조율 필요 ${adjustCount}` : '모두 가능'} </Chip> );frontend/src/features/candidate-schedule/model/index.ts (1)
24-24: Consider makingrequirementOfAdjustmentoptional.The
requirementOfAdjustmentfield is marked as required, but it might not always be needed for all participants. Consider making it optional to handle cases where adjustment information is not available.Apply this diff:
- requirementOfAdjustment: z.boolean(), + requirementOfAdjustment: z.boolean().optional(),frontend/src/features/candidate-schedule/ui/index.tsx (3)
12-12: Address the TODO comment about routing param location.Consider moving the routing parameter definitions to a dedicated routing configuration file for better organization and maintainability.
Would you like me to help create a routing configuration file and move these definitions there?
24-28: Enhance loading and error states.While the API integration is good, consider improving the user experience with:
- A proper loading component instead of plain text
- More informative error messages
- Error boundary implementation for graceful error handling
Example implementation:
- if (isPending) return <div>Pending...</div>; - if (!data) return <div>data is undefined or null</div>; + if (isPending) return <LoadingSpinner />; + if (!data) return ( + <ErrorMessage + message="Failed to load schedule details" + retry={() => queryClient.invalidateQueries({ queryKey: ['candidateDetail'] })} + /> + );
79-79: Consider explicit prop passing instead of spreading.While
{...props}works, explicitly passing required props to the Header component would make the requirements more obvious and maintainable.Example:
- <Header adjustCount={adjustCount} {...props} /> + <Header + adjustCount={adjustCount} + startDateTime={props.startDateTime} + endDateTime={props.endDateTime} + // Add other required props explicitly + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between a217af2dbbc3980759ab0bf84df4918d64822132 and 5cc20e7e20ad1bf5d6ba47fe5e1b16792ea82423.
📒 Files selected for processing (23)
frontend/src/features/candidate-schedule/api/index.ts(1 hunks)frontend/src/features/candidate-schedule/api/keys.ts(1 hunks)frontend/src/features/candidate-schedule/api/queries.ts(1 hunks)frontend/src/features/candidate-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/candidate-schedule/model/index.ts(1 hunks)frontend/src/features/candidate-schedule/ui/Header.tsx(4 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx(2 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts(4 hunks)frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/index.tsx(2 hunks)frontend/src/features/discussion/model/index.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCard/index.tsx(2 hunks)frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionTitle/index.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(3 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts(1 hunks)frontend/src/main.tsx(1 hunks)frontend/src/pages/CandidateSchedulePage/index.tsx(1 hunks)frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx(1 hunks)frontend/src/utils/date/format.ts(1 hunks)frontend/src/utils/zod/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- frontend/src/features/candidate-schedule/api/queries.ts
- frontend/src/features/candidate-schedule/api/keys.ts
- frontend/src/main.tsx
- frontend/src/utils/date/format.ts
- frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx
- frontend/src/features/discussion/model/index.ts
- frontend/src/features/discussion/ui/DiscussionTitle/index.tsx
- frontend/src/features/candidate-schedule/api/index.ts
- frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx
- frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts
- frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts
- frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts
- frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx
- frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx
🔇 Additional comments (7)
frontend/src/features/candidate-schedule/api/queryOptions.ts (1)
1-4: LGTM! Well-organized imports.The imports are properly structured with utilities and local modules clearly separated.
frontend/src/features/discussion/ui/DiscussionCard/index.tsx (3)
1-6: LGTM! Clean import organization.The new imports are well-organized and necessary for the added routing and context functionality.
19-29: LGTM! Clean routing implementation.The Link component is well-configured with appropriate params and state data for the candidate route.
17-17:Details
❓ Verification inconclusive
Add error handling for context usage.
The
useSafeContexthook might throw ifDiscussionContextis not provided. Consider adding error boundaries or fallback UI.
🏁 Script executed:
#!/bin/bash # Check if DiscussionContext.Provider is properly set up in parent components ast-grep --pattern $'<DiscussionContext.Provider value={$_}> $$$ <DiscussionCard $$$/> $$$ </DiscussionContext.Provider>'Length of output: 128
Action Required: Verify Context Provider and Implement Fallback
It appears that our search for a surrounding
<DiscussionContext.Provider>did not yield conclusive evidence thatDiscussionCardis always wrapped as expected. Given that theuseSafeContexthook will throw if the provider is missing, please double-check the parent component structure manually. In parallel, consider enhancing error handling by either wrapping the hook call in a try/catch block (or similar pattern) or providing a fallback UI to guard against potential runtime errors.
- Verify whether every
DiscussionCardis rendered within aDiscussionContext.Provider.- Implement error boundaries or conditional rendering to handle cases where the context might be undefined.
frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
1-14: LGTM! Well-organized imports and type definitions.The imports are properly structured with clear separation between external libraries, components, types, and utilities.
frontend/src/features/candidate-schedule/ui/index.tsx (2)
13-19: LGTM! Well-structured type definitions.The props interface is clear and includes all necessary fields for API integration. The type names are descriptive and follow TypeScript best practices.
21-32: LGTM! Clean API integration.The component successfully integrates with the API using the query hook. The data flow and prop handling are well-structured.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid type assertion for rank prop.
The type assertion rank as number could be unsafe. Consider handling the case where rank is undefined.
-<DiscussionLarge discussion={discussion} rank={rank as number} />
+<DiscussionLarge
+ discussion={discussion}
+ rank={rank ?? 0} // or handle undefined case appropriately
+/>📝 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.
| <DiscussionLarge discussion={discussion} rank={rank as number} /> | |
| <DiscussionLarge | |
| discussion={discussion} | |
| rank={rank ?? 0} // or handle undefined case appropriately | |
| /> |
There was a problem hiding this comment.
Remove hardcoded participant ID.
The selectedParticipantIds array is hardcoded to [1], which could cause issues if the ID doesn't match the actual user.
Consider using the actual participant IDs from the discussion or candidate data:
- selectedParticipantIds: [1],
+ selectedParticipantIds: candidate.usersForAdjust.map(user => user.id),📝 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 RecommendedScheduleItem = ({ | |
| candidate, discussionId, startDTStr, endDTStr, adjustCount, | |
| }: { | |
| candidate: DiscussionDTO; | |
| discussionId: number; | |
| startDTStr: string; | |
| endDTStr: string; | |
| adjustCount: number; | |
| }) => { | |
| const [startDT, endDT] = [new Date(startDTStr), new Date(endDTStr)]; | |
| return( | |
| <Link | |
| className={recommendItemStyle} | |
| params={{ id: discussionId.toString() }} | |
| state={{ candidate: { | |
| adjustCount: candidate.usersForAdjust.length, | |
| startDateTime: startDTStr, | |
| endDateTime: endDTStr, | |
| selectedParticipantIds: [1], | |
| } }} | |
| to={'/discussion/candidate/$id'} | |
| > | |
| 모두 가능 | |
| </Chip> | |
| </Flex> | |
| <Flex direction='column' gap={100}> | |
| <Text typo='b2M'>{formatKoreanDate(startDT, { year: true })}</Text> | |
| <Text color={vars.color.Ref.Netural[700]} typo='b3R'> | |
| {`${getTimeRangeString(startDT, endDT)} (${getHourDiff(startDT, endDT)}시간)`} | |
| </Text> | |
| </Flex > | |
| <AvailableChip adjustCount={adjustCount} /> | |
| </Link> | |
| ); | |
| }; | |
| const RecommendedScheduleItem = ({ | |
| candidate, discussionId, startDTStr, endDTStr, adjustCount, | |
| }: { | |
| candidate: DiscussionDTO; | |
| discussionId: number; | |
| startDTStr: string; | |
| endDTStr: string; | |
| adjustCount: number; | |
| }) => { | |
| const [startDT, endDT] = [new Date(startDTStr), new Date(endDTStr)]; | |
| return( | |
| <Link | |
| className={recommendItemStyle} | |
| params={{ id: discussionId.toString() }} | |
| state={{ candidate: { | |
| adjustCount: candidate.usersForAdjust.length, | |
| startDateTime: startDTStr, | |
| endDateTime: endDTStr, | |
| selectedParticipantIds: candidate.usersForAdjust.map(user => user.id), | |
| } }} | |
| to={'/discussion/candidate/$id'} | |
| > | |
| <Flex direction='column' gap={100}> | |
| <Text typo='b2M'>{formatKoreanDate(startDT, { year: true })}</Text> | |
| <Text color={vars.color.Ref.Netural[700]} typo='b3R'> | |
| {`${getTimeRangeString(startDT, endDT)} (${getHourDiff(startDT, endDT)}시간)`} | |
| </Text> | |
| </Flex > | |
| <AvailableChip adjustCount={adjustCount} /> | |
| </Link> | |
| ); | |
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Improve key prop implementation for better performance.
Using JSON.stringify(candidate) in the key prop is not recommended as it's computationally expensive and could cause issues with circular references.
Consider using a unique identifier from the candidate object:
- key={`${JSON.stringify(candidate)}-${idx}`}
+ key={`${candidate.id ?? idx}`}If the candidate doesn't have a unique ID, verify that one is available:
🏁 Script executed:
#!/bin/bash
# Check if candidate type has an id field
ast-grep --pattern 'interface DiscussionDTO {
$$$
id: $_
$$$
}'Length of output: 71
Improve key prop implementation to avoid performance issues.
The current implementation using JSON.stringify(candidate) for the key is both computationally expensive and can be error-prone (e.g., potential circular references). It's recommended to use a unique identifier from the candidate if available. For example, if the candidate objects include an id, you might update the key prop as follows:
- key={`${JSON.stringify(candidate)}-${idx}`}
+ key={candidate.id ?? idx}If there's any doubt about whether the candidate objects have a unique id, please verify the type definition for candidates manually or via additional queries to the codebase. This will ensure that the change doesn't introduce any unforeseen issues.
📝 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 RecommendedSchedules = ({ candidates, discussion }: { | |
| candidates: DiscussionCalendarResponse['events']; | |
| discussion: DiscussionResponse; | |
| }) => ( | |
| <Flex direction='column' width='full'> | |
| <Text className={recommendContainerStyle} typo='t2'>추천 일정</Text> | |
| <RecommendedScheduleItem /> | |
| <RecommendedScheduleItem /> | |
| <RecommendedScheduleItem /> | |
| {candidates.map((candidate, idx) => ( | |
| <RecommendedScheduleItem | |
| adjustCount={candidate.usersForAdjust.length} | |
| candidate={candidate} | |
| discussionId={discussion.id} | |
| endDTStr={candidate.endDateTime} | |
| key={`${JSON.stringify(candidate)}-${idx}`} | |
| startDTStr={candidate.startDateTime} | |
| /> | |
| ))} | |
| </Flex> | |
| ); | |
| const RecommendedSchedules = ({ candidates, discussion }: { | |
| candidates: DiscussionCalendarResponse['events']; | |
| discussion: DiscussionResponse; | |
| }) => ( | |
| <Flex direction='column' width='full'> | |
| <Text className={recommendContainerStyle} typo='t2'>추천 일정</Text> | |
| {candidates.map((candidate, idx) => ( | |
| <RecommendedScheduleItem | |
| adjustCount={candidate.usersForAdjust.length} | |
| candidate={candidate} | |
| discussionId={discussion.id} | |
| endDTStr={candidate.endDateTime} | |
| - key={`${JSON.stringify(candidate)}-${idx}`} | |
| + key={candidate.id ?? idx} | |
| startDTStr={candidate.startDateTime} | |
| /> | |
| ))} | |
| </Flex> | |
| ); |
frontend/src/utils/zod/index.ts
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using proper timezone handling.
The current implementation adds 9 hours to handle KST, but this approach might cause issues with daylight saving time (DST). Consider using a proper timezone library like date-fns-tz or luxon for robust timezone handling.
Example implementation using date-fns-tz:
-export const zCoerceToKSTDate = datelike.pipe(z.coerce.date()).transform((date) => {
- const kstDate = new Date(date);
- kstDate.setHours(kstDate.getHours() + 9);
- return kstDate;
-});
+import { zonedTimeToUtc } from 'date-fns-tz';
+
+export const zCoerceToKSTDate = datelike.pipe(z.coerce.date()).transform((date) => {
+ return zonedTimeToUtc(date, 'Asia/Seoul');
+});📝 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 zCoerceToKSTDate = datelike.pipe(z.coerce.date()).transform((date) => { | |
| const kstDate = new Date(date); | |
| kstDate.setHours(kstDate.getHours() + 9); | |
| return kstDate; | |
| }); | |
| import { zonedTimeToUtc } from 'date-fns-tz'; | |
| export const zCoerceToKSTDate = datelike.pipe(z.coerce.date()).transform((date) => { | |
| return zonedTimeToUtc(date, 'Asia/Seoul'); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/src/features/shared-schedule/model/upcomingSchedules.ts (2)
5-9: MovemeetingMethodOrLocationto SharedEventDtoSchema.The
meetingMethodOrLocationfield logically belongs to the shared event data but is currently defined inUpcomingScheduleSchema. Consider moving it toSharedEventDtoSchemafor better data organization and type safety.const SharedEventDtoSchema = z.object({ id: z.number(), startDateTime: zCoerceToDate, endDateTime: zCoerceToDate, + meetingMethodOrLocation: z.string(), });
11-17: Consider using an enum for meetingMethodOrLocation.Using a string type for
meetingMethodOrLocationreduces type safety. Consider using a Zod enum to restrict values to valid meeting methods.+const MeetingMethodSchema = z.enum(['ONLINE', 'OFFLINE']); + export const UpcomingScheduleSchema = z.object({ discussionId: z.number(), title: z.string(), - meetingMethodOrLocation: z.string(), + meetingMethodOrLocation: MeetingMethodSchema, sharedEventDto: SharedEventDtoSchema, participantPictureUrls: z.array(z.string()), });frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx (2)
18-19: Enhance error and loading states.The current implementation could be improved:
- Loading state shows a simple "pending..." text
- Empty state uses a separate component
Consider:
- Using a consistent approach for all states (loading, empty, error)
- Adding proper error handling
- Using a skeleton loader for better UX
if (isPending) return <UpcomingScheduleSkeleton />; if (error) return <UpcomingError error={error} />; if (schedules.length === 0) return <UpcomingFallback />;
22-23: Improve code comment documentation.The comment about relative positioning and overflow is important but could be more detailed.
Consider enhancing the comment:
/** * Note: The parent Flex component must not have position: relative * because the overflow: hidden needs to be applied at the viewport level * for proper carousel functionality. Setting relative positioning here * would create a new stacking context and break the carousel overflow. */frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx (1)
17-23: Consider adding error boundaries for schedule mapping.While the key change from
idtodiscussionIdis valid, consider wrapping the schedule mapping in an error boundary to gracefully handle cases wherediscussionIdmight be undefined, especially during API integration.{schedules.map((schedule) => ( + <ErrorBoundary key={schedule.discussionId} fallback={<ScheduleCardSkeleton />}> <ScheduleCard - key={schedule.discussionId} latest={false} schedule={schedule} /> + </ErrorBoundary> ))}frontend/src/features/candidate-schedule/ui/Header.tsx (1)
54-58: Enhance type safety with a dedicated interface.Consider extracting the props type into a dedicated interface for better maintainability and type safety.
+interface HeaderTextInfoProps { + adjustCount: number; + startTime: Date; + endTime: Date; +} + -const HeaderTextInfo = ({ adjustCount, startTime, endTime }: { - adjustCount: number; - startTime: Date; - endTime: Date; -}) => { +const HeaderTextInfo = ({ adjustCount, startTime, endTime }: HeaderTextInfoProps) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5cc20e7e20ad1bf5d6ba47fe5e1b16792ea82423 and b6c539a667dec7d7252faec2225cbb1af2be160c.
📒 Files selected for processing (11)
frontend/src/features/candidate-schedule/ui/Header.tsx(1 hunks)frontend/src/features/shared-schedule/model/index.ts(0 hunks)frontend/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts(0 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx(2 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/utils/zod/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- frontend/src/features/shared-schedule/model/index.ts
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/utils/zod/index.ts
🔇 Additional comments (10)
frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1)
19-24: LGTM!The response schema and type definitions are well-structured, following Zod best practices with proper type inference.
frontend/src/pages/UpcomingSchedulePage/index.css.ts (1)
10-10: Verify the visual impact of spacing changes.The significant reduction in gap (from 120 to 32) combined with the new marginTop might affect the component spacing and overall layout. Please ensure these changes:
- Don't create unintended visual clutter due to reduced spacing
- Maintain proper visual hierarchy
- Align with the design specifications
Also applies to: 14-14
frontend/src/pages/UpcomingSchedulePage/index.tsx (2)
10-10: Consider using proper mock data structure.The schedules array contains empty objects which might not represent the actual data structure expected by the UpcomingScheduleList component.
Consider using mock data that matches the expected schema:
const schedules = [ { id: 1, title: 'Mock Schedule 1' }, { id: 2, title: 'Mock Schedule 2' }, // ... more mock data with proper structure ];
17-25: Verify the large gap value in Flex components.Both outer and inner Flex components use a gap of 700, which seems unusually large. This might create excessive vertical spacing.
Consider:
- Using a smaller gap value that matches the design
- Using consistent units (px, rem, etc.) across the application
- Documenting the reason for such large spacing if intentional
frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx (3)
54-66: LGTM! Clean implementation of the DdayChip component.The component has well-defined props with proper typing and follows React best practices.
68-75: LGTM! Clean implementation of the MeetingDateTimeInfo component.The component has well-defined props with proper typing and follows React best practices.
25-25:Details
❓ Verification inconclusive
Verify type definition for the new data structure.
The code now accesses date-time properties through
schedule.sharedEventDto. Let's ensure type safety and improve developer experience.Run this script to check the type definition:
Additionally, consider adding type assertions or runtime checks to handle potential undefined values:
// Add type guard const hasSharedEventDto = (schedule: UpcomingSchedule): schedule is UpcomingSchedule & { sharedEventDto: { startDateTime: Date; endDateTime: Date } } => { return 'sharedEventDto' in schedule && 'startDateTime' in schedule.sharedEventDto && 'endDateTime' in schedule.sharedEventDto; };Also applies to: 29-30
🏁 Script executed:
#!/bin/bash # Description: Check the UpcomingSchedule type definition ast-grep --pattern 'type UpcomingSchedule = { $$$ sharedEventDto: { $$$ startDateTime: $_ endDateTime: $_ $$$ } $$$ }'Length of output: 144
Review: Verify UpcomingSchedule Type Definition and Consider Type Guarding
It appears that our preliminary automated scan did not locate an exact match for the expected type structure (i.e. a nested
sharedEventDtowithstartDateTimeandendDateTime). Please manually verify that theUpcomingScheduletype definition (likely in your models) accurately reflects these properties. In addition, consider adding a type guard (as shown below) to ensure that the values are defined at runtime:
- Action items:
- Confirm that the
UpcomingScheduletype includes asharedEventDtostructured with bothstartDateTimeandendDateTime.- Consider incorporating type assertions or runtime type guards to handle potential missing values.
// Example type guard for UpcomingSchedule const hasSharedEventDto = (schedule: UpcomingSchedule): schedule is UpcomingSchedule & { sharedEventDto: { startDateTime: Date; endDateTime: Date } } => { return schedule.sharedEventDto !== undefined && schedule.sharedEventDto.startDateTime !== undefined && schedule.sharedEventDto.endDateTime !== undefined; };Please review the type definition manually, and update if necessary, to ensure full type safety in this API integration.
frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx (1)
19-19:Details
✅ Verification successful
Verify the type definition includes discussionId property.
The key prop has been changed from
idtodiscussionId. Let's verify that theUpcomingScheduletype definition includes this property to ensure type safety.
🏁 Script executed:
#!/bin/bash # Search for the UpcomingSchedule type definition ast-grep --pattern 'type UpcomingSchedule = { $$$ discussionId: $_ $$$ }' # Alternative search if it's an interface ast-grep --pattern 'interface UpcomingSchedule { $$$ discussionId: $_ $$$ }'Length of output: 166
Let's run another verification script to search the entire repository for occurrences of the
UpcomingScheduletype/interface and check for thediscussionIdproperty. This will help us confirm whether the type definition includes thediscussionIdproperty as expected.Please execute this script and provide the output so we can further verify the type definition before finalizing the review comment.
🏁 Script executed:
#!/bin/bash echo "Searching for UpcomingSchedule type/interface definitions..." rg -t ts -n "UpcomingSchedule" . echo "" echo "Searching for 'discussionId' occurrences..." rg -t ts -n "discussionId" .Length of output: 13777
Confirmed: The UpcomingSchedule type correctly includes the discussionId property.
- The search results in
frontend/src/features/shared-schedule/model/upcomingSchedules.tsconfirm thatdiscussionIdis defined as az.number()within the schema.- The key prop change to
schedule.discussionIdinfrontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsxis consistent with the type definition and maintains type safety and proper React reconciliation.frontend/src/pages/HomePage/index.tsx (1)
17-33: LGTM! Layout improvements enhance the visual hierarchy.The changes improve the layout structure by:
- Using
justify='space-between'andwidth='full'for better spacing- Wrapping the title and button in a separate Flex container for proper alignment
- Moving
UpcomingSchedulesoutside for better visual separationfrontend/src/features/candidate-schedule/ui/Header.tsx (1)
8-14: Fix inconsistent prop names in HeaderTextInfo usage.The props have been renamed to
startDateTimeandendDateTime, but they are passed asstartTimeandendTimeto HeaderTextInfo.Apply this diff to maintain consistency:
<HeaderTextInfo adjustCount={adjustCount} - endTime={endDateTime} - startTime={startDateTime} + endDateTime={endDateTime} + startDateTime={startDateTime} />Also applies to: 23-27
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the TODO comment about date management.
The TODO comment indicates inconsistency in date management across the codebase. Consider standardizing the date handling approach.
Let's check the date handling patterns in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for date-related type definitions and usage patterns
rg -g '*.ts*' -A 3 'type.*Date|interface.*Date|Date\['Length of output: 14749
Unify Date Management Across the Codebase
The Header component currently converts dates to strings using formatDateToDateTimeString when calling the mutation. However, our investigation shows that while many UI components (e.g., in the DatePicker and Calendar contexts) handle dates using native Date objects, some API models (like in my-calendar/model/index.ts) expect dates as strings. To avoid potential technical debt, please review and standardize the date handling approach. Consider one of the following strategies:
- Centralize Conversion: Keep using native
Dateobjects in the UI and perform a consistent conversion to string at the API boundary. - Unified Model: Alternatively, refactor the API models to accept
Dateobjects if that aligns with the overall application design.
Please adjust the date management strategy accordingly to ensure consistency.
dioo1461
force-pushed
the
feature/fe/candidate-detail-api
branch
from
b6c539a to
7556366
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
45-50:⚠️ Potential issueRemove non-null assertion and handle undefined case properly.
The TODO comment indicates that undefined case handling is needed, but the code uses a non-null assertion (
!) which bypasses TypeScript's null checks. This is inconsistent with the TODO and could lead to runtime errors.The undefined case is already handled by the check on line 36, making the non-null assertion unnecessary.
- {/* TODO: candidate 가 undefined일 경우의 예외 처리 */} <RecommendedSchedules - candidates={candidates!} + candidates={candidates} discussion={discussion} participants={participants} />frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
25-35: 🛠️ Refactor suggestionImprove key prop implementation to avoid performance issues.
Using
JSON.stringify(candidate)for the key is computationally expensive and can cause issues with circular references.<RecommendedScheduleItem adjustCount={candidate.usersForAdjust.length} candidate={candidate} discussionId={discussion.id} endDTStr={candidate.endDateTime} - key={`${JSON.stringify(candidate)}-${idx}`} + key={candidate.id ?? idx} participants={participants} startDTStr={candidate.startDateTime} />frontend/src/features/candidate-schedule/ui/Header.tsx (1)
28-32:⚠️ Potential issueFix inconsistent prop names in HeaderTextInfo usage.
The props have been renamed to
startDateTimeandendDateTime, but they are passed asstartTimeandendTimeto HeaderTextInfo.Apply this diff to maintain consistency:
<HeaderTextInfo adjustCount={adjustCount} - endTime={endDateTime} - startTime={startDateTime} + endDateTime={endDateTime} + startDateTime={startDateTime} />
🧹 Nitpick comments (13)
frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1)
11-17: Consider using an enum for meetingMethodOrLocation.While the schema structure is good, consider maintaining type safety by using an enum for
meetingMethodOrLocationinstead of a generic string.- meetingMethodOrLocation: z.string(), + meetingMethodOrLocation: z.enum(['ONLINE', 'OFFLINE']),frontend/src/features/candidate-schedule/api/keys.ts (1)
3-11: Consider sorting selectedUserIdList for stable query keys.To ensure consistent cache behavior, consider sorting the selectedUserIdList array to avoid cache misses when the same users are selected in a different order.
export const candidateDetailQueryKey = { all: [BASE_KEY], detail: ( discussionId: number, startDateTime: string, endDateTime: string, selectedUserIdList: number[], - ) => [BASE_KEY, discussionId, startDateTime, endDateTime, selectedUserIdList], + ) => [BASE_KEY, discussionId, startDateTime, endDateTime, [...selectedUserIdList].sort()], };frontend/src/features/candidate-schedule/api/queryOptions.ts (1)
6-19: Consider increasing the cache time.The current cache time of 1 minute seems short for candidate schedule data that likely doesn't change frequently. Consider increasing it to reduce unnecessary API calls and improve performance.
export const candidateDetailQueryOption = ( discussionId: number, startDateTime: string, endDateTime: string, selectedUserIdList: number[], ) => ({ queryKey: candidateDetailQueryKey.detail( discussionId, startDateTime, endDateTime, selectedUserIdList, ), queryFn: () => candidateDetailApi.getCandidateScheduleDetail( discussionId, startDateTime, endDateTime, selectedUserIdList, ), - cacheTime: 1 * MINUTE_IN_MILLISECONDS, + cacheTime: 5 * MINUTE_IN_MILLISECONDS, // Increase cache time to 5 minutes });frontend/src/features/shared-schedule/api/keys.ts (1)
3-15: LGTM! The query key restructuring improves modularity.The separation into distinct exports with type-safe detail functions enhances code organization and maintainability.
Consider adding a shared type for the query key structure to reduce duplication:
type QueryKey<T extends unknown[]> = { all: string[]; detail: (...args: T) => (string | number)[]; }; export const ongoingQueryKey: QueryKey<[number, number, AttendType]> = { all: ['ongoing'], detail: (page, size, type) => [...ongoingQueryKey.all, page, size, type], };frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx (1)
22-29: Consider extracting magic numbers and documenting layout constraints.The height value and flex-end justification appear to be specific layout requirements.
Consider extracting the height constant and documenting the layout constraints:
+const UPCOMING_SCHEDULES_HEIGHT = 358; + const UpcomingSchedules = () => { // ... return ( // 최상단 Flex에 relative 주면 안 됨! (overflow hidden 때문에 뷰포트가 부모여야 함) <Flex direction='column' gap={700} - height={358} + height={UPCOMING_SCHEDULES_HEIGHT} justify='flex-end' width='full' >frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx (1)
53-54: Address TODO comment about segment option type generics.The TODO comment indicates a need for generic type implementation to avoid type assertion.
Would you like me to help implement generic types for the segment options to eliminate the type assertion? This would improve type safety and maintainability.
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
25-33: Extract discussionId conversion to avoid duplication.The
discussionId.toString()is called multiple times. Consider extracting it to a variable.const ScheduleContents = ({ discussionId }: ScheduleDetailsProps) => { + const discussionIdStr = discussionId.toString(); const { discussion, isLoading: isDiscussionLoading } = useDiscussionQuery( - discussionId.toString(), + discussionIdStr, ); const { participants, isPending: isParticipantsLoading } = - useDiscussionParticipantsQuery(discussionId.toString()); + useDiscussionParticipantsQuery(discussionIdStr); const { calendar: candidates, isPending: isCandidateLoading } = useDiscussionCalendarQuery( - discussionId.toString(), + discussionIdStr, { size: 3 }, );frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
39-51: Optimize Date object creation.Creating Date objects in the render function can impact performance. Consider using useMemo to memoize these calculations.
const RecommendedScheduleItem = ({ candidate, discussionId, startDTStr, endDTStr, adjustCount, participants, }: { candidate: DiscussionDTO; discussionId: number; startDTStr: string; endDTStr: string; adjustCount: number; participants: UserDTO[]; }) => { - const [startDT, endDT] = [new Date(startDTStr), new Date(endDTStr)]; - const [startTime, endTime] = [getTimeParts(startDT), getTimeParts(endDT)]; + const [startDT, endDT, startTime, endTime] = useMemo(() => { + const [start, end] = [new Date(startDTStr), new Date(endDTStr)]; + return [start, end, getTimeParts(start), getTimeParts(end)]; + }, [startDTStr, endDTStr]);frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx (1)
6-9: Consider providing a more user-friendly fallback.
Currently, the code simply returns a<div>stating “candidate is undefined or null.” Providing a more descriptive or styled fallback message can improve usability (e.g., a 404-style page or redirecting the user to a safe screen).frontend/src/pages/CandidateSchedulePage/index.tsx (1)
7-12: Refactor the function signature for clarity.
Accepting an unnamed parameter object namedcandidatemight be confusing. Consider using a named prop, e.g.(props: { candidate: CandidateDetailResponse })or destructuring for clarity:-const CandidateSchedulePage = (candidate: { adjustCount: number; ... }) => { +interface CandidateProps { + candidate: { + adjustCount: number; + // ... + } +} +const CandidateSchedulePage = ({ candidate }: CandidateProps) => {frontend/src/features/candidate-schedule/model/index.ts (1)
42-42: Rename theScheduleEventZod object for clarity.
Using the same identifier for both the Zod schema and the inferred type (i.e.,ScheduleEvent) may cause naming confusion. A clearer approach is to rename the Zod object (e.g.,ScheduleEventSchema) and keepScheduleEventfor the inferred TypeScript type.frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts (1)
87-87: Remove or uncomment the maxWidth property.The commented-out
maxWidth: '100%'property suggests an unfinished change. Either remove it if not needed or uncomment it if the feature is required.frontend/src/features/candidate-schedule/ui/index.tsx (1)
26-30: Enhance loading state UI.While the loading state is handled, consider using a proper loading component instead of a simple div for better user experience.
- if (isPending) return <div>Pending...</div>; - if (!data) return <div>data is undefined or null</div>; + if (isPending) return <LoadingSpinner />; + if (!data) return <ErrorState message="Failed to load data" />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between b6c539a667dec7d7252faec2225cbb1af2be160c and 7556366063eaff92c1577a4e40013001cd0a5ca4.
📒 Files selected for processing (37)
frontend/src/features/candidate-schedule/api/index.ts(1 hunks)frontend/src/features/candidate-schedule/api/keys.ts(1 hunks)frontend/src/features/candidate-schedule/api/queries.ts(1 hunks)frontend/src/features/candidate-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/candidate-schedule/model/index.ts(1 hunks)frontend/src/features/candidate-schedule/ui/Header.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx(2 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts(4 hunks)frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/index.tsx(1 hunks)frontend/src/features/discussion/model/index.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCard/index.tsx(2 hunks)frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionTitle/index.tsx(1 hunks)frontend/src/features/shared-schedule/api/keys.ts(1 hunks)frontend/src/features/shared-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/shared-schedule/model/index.ts(0 hunks)frontend/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(3 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx(2 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts(0 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx(2 hunks)frontend/src/main.tsx(1 hunks)frontend/src/pages/CandidateSchedulePage/index.tsx(1 hunks)frontend/src/pages/DiscussionPage/MemberContext.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/$id.lazy.tsx(1 hunks)frontend/src/routes/_main/discussion/candidate/index.lazy.tsx(0 hunks)frontend/src/utils/date/format.ts(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/features/shared-schedule/model/index.ts
- frontend/src/routes/_main/discussion/candidate/index.lazy.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/pages/DiscussionPage/MemberContext.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- frontend/src/pages/UpcomingSchedulePage/index.css.ts
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx
- frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx
- frontend/src/utils/date/format.ts
- frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts
- frontend/src/main.tsx
- frontend/src/features/candidate-schedule/api/index.ts
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx
- frontend/src/features/discussion/model/index.ts
- frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx
- frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx
- frontend/src/features/discussion/ui/DiscussionCard/index.tsx
- frontend/src/features/discussion/ui/DiscussionTitle/index.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts
- frontend/src/pages/UpcomingSchedulePage/index.tsx
- frontend/src/pages/HomePage/index.tsx
- frontend/src/features/candidate-schedule/api/queries.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx (1)
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#205
File: frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx:29-33
Timestamp: 2025-02-19T05:54:56.987Z
Learning: The hardcoded selectedUserIdList in CalendarTable.tsx will be replaced with dynamic user selection when checkbox functionality is implemented in a future PR.
🔇 Additional comments (20)
frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1)
5-9: LGTM! Well-structured schema definition.The
SharedEventDtoSchemais well-designed with appropriate types and proper date handling usingzCoerceToDate.frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx (1)
34-34: LGTM! Good use of nullish coalescing operator.The change from
|| nullto?? undefinedis a good improvement as it better handles the optional nature ofselectedUserIdListin TypeScript. This aligns with the plan to support dynamic user selection.frontend/src/features/shared-schedule/api/queryOptions.ts (1)
4-4: LGTM! Query options correctly updated to use new query key structure.The changes maintain type safety while improving code organization through explicit imports and consistent usage of the new query key structure.
Also applies to: 8-8, 12-12, 16-16
frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx (1)
10-10: LGTM! Component signature simplified.Removal of unused children prop improves component focus.
frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx (1)
9-9: LGTM! Query key usage updated correctly.The changes properly implement the new query key structure.
Also applies to: 44-44
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
5-9: LGTM!The query hooks are correctly imported and well-organized.
frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
74-83: LGTM!The AvailableChip component is well-implemented with clear conditional logic for color and text based on adjustCount.
frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx (2)
11-16: Guard against mismatched candidate props.
SinceCandidateSchedulePageis spread with...candidate, ensure that all required prop fields are present and properly typed. If any field is missing oncandidate, it may cause runtime issues.
19-21: Route definition looks correct.
The dynamic route$idis properly configured withcreateLazyFileRoute, and the component reference aligns with the desired screen.frontend/src/pages/CandidateSchedulePage/index.tsx (1)
14-14: Implement error handling for missing candidate data.
This comment mirrors a past remark about adding error boundaries or fallback UI to handle undefined candidates.frontend/src/features/candidate-schedule/model/index.ts (2)
3-3: Validate consistency between request and response date fields.
SinceCandidateDetailResponseSchemauseszCoerceToDatebutCandidateDetailRequestSchemahas raw string fields, ensure that the code correctly handles parsing on both ends or consider usingzCoerceToDatein the request schema as well if you want typed Date objects.
19-25: Recommend making optional fields visibly optional in all references.
By default,selectedis optional, andrequirementOfAdjustmentis required. If the backend can omitrequirementOfAdjustmentin some scenarios, consider making it optional or providing a default.frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts (2)
11-11: LGTM! Consistent minimum height ensures stable layout.The addition of
minHeight: '23.25rem'to bothtimelineCanvasStyleandtimelineBlockContainerStyleensures a consistent layout and prevents unwanted resizing.Also applies to: 48-48
68-79: LGTM! Improved variant naming convention.The updated variant names (
ADJUSTABLE,FIXED,OUT_OF_RANGE) follow a consistent uppercase naming convention, improving code readability and maintainability.frontend/src/features/candidate-schedule/ui/Header.tsx (3)
11-16: LGTM! Well-defined interface with proper typing.The
HeaderPropsinterface clearly defines the required props with appropriate types.
34-34: Unify Date Management Across the CodebaseThe TODO comment indicates inconsistency in date management across the codebase. Consider standardizing the date handling approach.
Let's check the date handling patterns in the codebase:
#!/bin/bash # Search for date-related type definitions and usage patterns rg -g '*.ts*' -A 3 'type.*Date|interface.*Date|Date\['
35-51: LGTM! Well-implemented mutation handling with proper query invalidation.The mutation logic is well-implemented with:
- Proper error handling
- Query invalidation for both upcoming and ongoing queries
- Clear button text
frontend/src/features/candidate-schedule/ui/index.tsx (3)
15-21: LGTM! Well-defined props interface.The
CandidateScheduleDetailPropsinterface clearly defines all required props with appropriate types.
49-68: LGTM! Well-implemented navigation with proper checks.The TopBar component correctly implements navigation with:
- Proper router usage
- Back navigation capability check
- Clear click handler
70-74: LGTM! Well-structured Content props interface.The
ContentPropsinterface properly extendsCandidateDetailResponseand includes additional required props.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for query failures.
The code only handles loading and empty states. Consider adding error handling to improve user experience.
if (isDiscussionLoading || isCandidateLoading || isParticipantsLoading)
return <div>pending ...</div>;
+ if (discussion?.error || candidates?.error || participants?.error)
+ return <div>Failed to load data. Please try again.</div>;
if (!discussion || !candidates || !participants) return <div>No data available</div>;📝 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 (isDiscussionLoading || isCandidateLoading || isParticipantsLoading) | |
| return <div>pending ...</div>; | |
| if (!discussion || !candidates || !participants) return <div>No data available</div>; | |
| if (isDiscussionLoading || isCandidateLoading || isParticipantsLoading) | |
| return <div>pending ...</div>; | |
| if (discussion?.error || candidates?.error || participants?.error) | |
| return <div>Failed to load data. Please try again.</div>; | |
| if (!discussion || !candidates || !participants) return <div>No data available</div>; |
dioo1461
force-pushed
the
feature/fe/candidate-detail-api
branch
from
7556366 to
e06a14f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
frontend/src/features/candidate-schedule/ui/Header.tsx (1)
28-32:⚠️ Potential issueFix inconsistent prop names in HeaderTextInfo.
The props have been renamed to
startDateTimeandendDateTime, but they are passed asstartTimeandendTimeto HeaderTextInfo.Apply this diff to maintain consistency:
<HeaderTextInfo adjustCount={adjustCount} - endTime={endDateTime} - startTime={startDateTime} + endDateTime={endDateTime} + startDateTime={startDateTime} />
🧹 Nitpick comments (7)
frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingScheduleList.tsx (1)
13-13: Address the TODO comment about infinite scroll implementation.The comment indicates a pending feature that could improve performance with large datasets.
Would you like me to help implement infinite scroll using a library like
react-infinite-scroll-componentorreact-window?frontend/src/pages/UpcomingSchedulePage/index.tsx (2)
11-14: Enhance loading and error states.Consider using a proper loading component and more informative error messages for better user experience.
- if (isPending) return <div>Pending...</div>; - if (!data) return <div>data is undefined or null</div>; + if (isPending) return <LoadingSpinner />; + if (!data) return <ErrorMessage message="Failed to load upcoming schedules" />;
28-30: Consider extracting schedule splitting logic.The slice operations could be moved to a utility function or custom hook for better reusability.
+const CAROUSEL_LIMIT = 3; + +const useScheduleSplit = (schedules: UpcomingSchedule[]) => ({ + carouselSchedules: schedules.slice(0, CAROUSEL_LIMIT), + listSchedules: schedules.slice(CAROUSEL_LIMIT), +});frontend/src/features/candidate-schedule/model/index.ts (1)
28-32: Consider using zCoerceToDate for request schema.The request schema uses string for dates while the response schema uses zCoerceToDate. Consider using consistent date handling.
- startDateTime: z.string(), - endDateTime: z.string(), + startDateTime: zCoerceToDate, + endDateTime: zCoerceToDate,frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
25-33: Consider using a custom hook for data fetching.The component is using multiple query hooks which could be consolidated into a single custom hook for better reusability and cleaner component logic.
+const useScheduleData = (discussionId: string) => { + const { discussion, isLoading: isDiscussionLoading } = useDiscussionQuery(discussionId); + const { participants, isPending: isParticipantsLoading } = useDiscussionParticipantsQuery(discussionId); + const { calendar: candidates, isPending: isCandidateLoading } = useDiscussionCalendarQuery( + discussionId, + { size: 3 }, + ); + + return { + discussion, + participants, + candidates, + isLoading: isDiscussionLoading || isCandidateLoading || isParticipantsLoading, + }; +};frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
52-62: Consider extracting link state preparation to a helper function.The link state preparation logic could be moved to a helper function for better readability and reusability.
+const prepareCandidateState = (candidate: DiscussionDTO, startDTStr: string, endDTStr: string, participants: UserDTO[]) => ({ + candidate: { + adjustCount: candidate.usersForAdjust.length, + startDateTime: startDTStr, + endDateTime: endDTStr, + selectedParticipantIds: participants.map(v => v.id), + } +}); <Link className={recommendItemStyle} params={{ id: discussionId.toString() }} - state={{ candidate: { - adjustCount: candidate.usersForAdjust.length, - startDateTime: startDTStr, - endDateTime: endDTStr, - selectedParticipantIds: participants.map(v => v.id), - } }} + state={prepareCandidateState(candidate, startDTStr, endDTStr, participants)} to={'/discussion/candidate/$id'} >frontend/src/features/candidate-schedule/ui/index.tsx (1)
14-14: Move routing param definitions to a dedicated route configuration file.The TODO comment suggests moving routing param definitions. This would improve code organization and maintainability.
Consider creating a dedicated route configuration file (e.g.,
routes.ts) and moving the param definitions there:// routes.ts export interface CandidateRouteParams { discussionId: string; } export const candidateRoute = { path: '/_main/discussion/candidate/$id', params: {} as CandidateRouteParams, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7556366063eaff92c1577a4e40013001cd0a5ca4 and e06a14f77c47cd0ea08d18c00e19c8c3ed5ab86b.
📒 Files selected for processing (39)
frontend/src/features/candidate-schedule/api/index.ts(1 hunks)frontend/src/features/candidate-schedule/api/keys.ts(1 hunks)frontend/src/features/candidate-schedule/api/queries.ts(1 hunks)frontend/src/features/candidate-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/candidate-schedule/model/index.ts(1 hunks)frontend/src/features/candidate-schedule/ui/Header.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx(2 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts(4 hunks)frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/index.tsx(1 hunks)frontend/src/features/discussion/model/index.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCard/index.tsx(2 hunks)frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionTitle/index.tsx(1 hunks)frontend/src/features/shared-schedule/api/keys.ts(1 hunks)frontend/src/features/shared-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/shared-schedule/model/index.ts(0 hunks)frontend/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx(4 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(3 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx(2 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx(3 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingScheduleList.tsx(2 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts(0 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx(2 hunks)frontend/src/main.tsx(1 hunks)frontend/src/pages/CandidateSchedulePage/index.tsx(1 hunks)frontend/src/pages/DiscussionPage/MemberContext.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/$id.lazy.tsx(1 hunks)frontend/src/routes/_main/discussion/candidate/index.lazy.tsx(0 hunks)frontend/src/utils/date/format.ts(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/routes/_main/discussion/candidate/index.lazy.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts
- frontend/src/features/shared-schedule/model/index.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- frontend/src/pages/DiscussionPage/MemberContext.ts
- frontend/src/features/discussion/ui/DiscussionTitle/index.tsx
- frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx
- frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx
- frontend/src/pages/HomePage/index.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts
- frontend/src/features/candidate-schedule/api/queryOptions.ts
- frontend/src/features/candidate-schedule/api/index.ts
- frontend/src/main.tsx
- frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts
- frontend/src/features/candidate-schedule/api/queries.ts
- frontend/src/features/candidate-schedule/api/keys.ts
- frontend/src/utils/date/format.ts
- frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx
- frontend/src/features/discussion/ui/DiscussionCard/index.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx
- frontend/src/pages/UpcomingSchedulePage/index.css.ts
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx
- frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx
- frontend/src/features/shared-schedule/api/keys.ts
- frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx
- frontend/src/features/discussion/model/index.ts
- frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts
🔇 Additional comments (17)
frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (4)
1-14: LGTM! Clean import organization and improved constant naming.The addition of
useEffectimport and the renaming of the constant toPAGE_SIZEimproves code clarity and reusability.
25-33: LGTM! Robust selection management logic.The new
useEffecthook ensures that the selected discussion ID always corresponds to an existing item in the current data, with a sensible fallback to the first item.
63-87: LGTM! Well-structured component extraction with strong typing.The extraction of
ScheduleItemsinto a separate component with proper TypeScript types improves code organization and maintainability. The component is purely presentational, following good separation of concerns.
89-90: LGTM! Clean export structure.The static Item property and default export follow React component conventions and provide a clean API for consuming components.
frontend/src/features/shared-schedule/api/queryOptions.ts (2)
4-4: LGTM! Import restructuring improves modularity.The transition to individual query key imports enhances code organization and potentially enables better tree-shaking.
8-8:Details
✅ Verification successful
Verify query key structure consistency.
The query key updates follow a consistent pattern, with
.detail()used for parameterized queries. Please ensure this pattern is maintained across other components using these query keys.Also applies to: 12-12, 16-16
🏁 Script executed:
#!/bin/bash # Description: Check for consistent usage of query keys across the codebase # Check for any inconsistent usage of the old query key structure rg -l "sharedScheduleQuerykeys" frontend/ # Check for consistent usage of .detail() with the new query keys rg -A 2 "ongoingQueryKey|finishedQueryKey" frontend/Length of output: 3278
Query key structure consistency verified.
The updated query key definitions and their usage—specifically the use of
.detail()for parameterized queries—are applied consistently across the codebase. For example:
- In
frontend/src/features/shared-schedule/api/keys.ts, bothongoingQueryKeyandfinishedQueryKeycorrectly implement thedetailfunction with the appropriate parameters.- In
frontend/src/features/shared-schedule/api/queryOptions.ts, the import and usage ofupcomingQueryKey, along with the parameterized usage ofongoingQueryKeyandfinishedQueryKey, align with the new patterns.- Other components (e.g., in
Header.tsxandOngoingSchedules/index.tsx) continue to use the keys appropriately, with the expected spread of.allfor invalidation and.detail()for fetching specific slices.Please ensure that any future query key references continue to adhere to this structure.
frontend/src/features/shared-schedule/model/upcomingSchedules.ts (2)
5-9: LGTM! Good modularization of shared event properties.The introduction of
SharedEventDtoSchemaimproves code reusability by extracting common event properties.
11-17:Details
❓ Verification inconclusive
Verify the removal of meeting method enum constraint.
The
meetingMethodOrLocationfield no longer validates against specific values ('ONLINE', 'OFFLINE'). This might lead to inconsistent data if the API accepts arbitrary strings.
🏁 Script executed:
#!/bin/bash # Search for meeting method values in the codebase rg -A 2 "meetingMethodOrLocation.*=.*['\"]"Length of output: 47
Attention: Verify Intentional Removal of Enum Constraint for 'meetingMethodOrLocation'
The schema now defines
meetingMethodOrLocationas a plain string:export const UpcomingScheduleSchema = z.object({ discussionId: z.number(), title: z.string(), meetingMethodOrLocation: z.string(), sharedEventDto: SharedEventDtoSchema, participantPictureUrls: z.array(z.string()), });This change removes the previous restriction to values like
"ONLINE"or"OFFLINE". Please manually verify that this change was deliberate and that the API or downstream processes can reliably handle arbitrary strings without introducing inconsistencies.frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingScheduleList.tsx (2)
9-11: LGTM! Improved type safety with UpcomingSchedule[].The prop type update enhances type checking and provides better IDE support.
26-33: LGTM! Improved schedule item rendering.Good use of schedule properties and unique keys with discussionId.
frontend/src/features/candidate-schedule/model/index.ts (1)
5-9: LGTM! Consistent enum values.Good update to use uppercase for status literals, following enum naming conventions.
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (2)
5-9: LGTM! Clean import organization.The imports are well-organized, with related query hooks grouped together.
45-50: Remove non-null assertion and handle undefined case properly.The TODO comment indicates that undefined case handling is needed, but the code uses a non-null assertion (
!) which bypasses TypeScript's null checks.- {/* TODO: candidate 가 undefined일 경우의 예외 처리 */} - <RecommendedSchedules - candidates={candidates!} - discussion={discussion} - participants={participants} - /> + {candidates && ( + <RecommendedSchedules + candidates={candidates} + discussion={discussion} + participants={participants} + /> + )}frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (3)
18-22: LGTM! Well-typed props interface.The props are properly typed using TypeScript, making the component's interface clear and type-safe.
25-35: Improve key prop implementation to avoid performance issues.Using
JSON.stringify(candidate)for the key prop is computationally expensive and can be error-prone.- key={`${JSON.stringify(candidate)}-${idx}`} + key={candidate.id ?? idx}
74-83: LGTM! Clean implementation of AvailableChip component.The component has a clear, single responsibility and uses proper prop typing.
frontend/src/pages/CandidateSchedulePage/index.tsx (1)
14-14: Implement error handling for missing candidate data.The TODO comment indicates missing error handling. This should be addressed to prevent potential runtime errors.
Consider implementing proper error boundaries or fallback UI components to handle cases where candidate data is missing.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array index as key prop.
Using array indices as keys can lead to issues with component rerendering and state management, especially if the list items can be reordered or filtered.
Use a unique identifier instead:
- key={index}
+ key={schedule.discussionId}📝 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.
| {schedules.map((schedule, index) => ( | |
| <OngoingScheduleListItem | |
| key={index} | |
| onSelect={onSelect} | |
| schedule={schedule} | |
| selected={selectedId === schedule.discussionId} | |
| /> | |
| ))} | |
| {schedules.map((schedule, index) => ( | |
| <OngoingScheduleListItem | |
| key={schedule.discussionId} | |
| onSelect={onSelect} | |
| schedule={schedule} | |
| selected={selectedId === schedule.discussionId} | |
| /> | |
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type guards and error handling for candidate props.
The component accepts candidate data but lacks proper type guards and error handling. This could lead to runtime errors if the data is malformed.
Apply this diff to add proper type guards and error handling:
const CandidateSchedulePage = (candidate: {
adjustCount: number;
startDateTime: string;
endDateTime: string;
selectedParticipantIds: number[];
}) => {
const { id } = useParams({ from: '/_main/discussion/candidate/$id' });
+ if (!candidate || !id) {
+ return <div>Invalid candidate data or missing ID</div>;
+ }
+
return (
<>
<div className={backdropStyle} />
- <CandidateScheduleDetail discussionId={Number(id)} {...candidate} />
+ <CandidateScheduleDetail
+ discussionId={Number(id)}
+ {...candidate}
+ />
</>
);
};Also applies to: 19-19
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize date management across the codebase.
The TODO comment indicates inconsistency in date handling. Consider adopting a consistent approach:
- Use
Dateobjects internally - Convert to strings only at API boundaries
- Consider using a date management library like
date-fnsorluxon
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and user feedback for mutation.
The mutation lacks error handling and user feedback. Users won't know if the schedule confirmation succeeded or failed.
Apply this diff to add proper error handling and feedback:
<Button
onClick={() => {
mutate({
id: discussionId.toString(),
body: {
startDateTime: formatDateToDateTimeString(startDateTime),
endDateTime: formatDateToDateTimeString(endDateTime),
},
+ }, {
+ onSuccess: () => {
+ queryClient.invalidateQueries({ queryKey: upcomingQueryKey });
+ queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all });
+ // Add success toast/notification here
+ },
+ onError: (error) => {
+ console.error('Failed to confirm schedule:', error);
+ // Add error toast/notification here
+ },
});
- queryClient.invalidateQueries({ queryKey: upcomingQueryKey });
- queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all });
}}
size='lg'
>📝 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.
| onClick={() => { | |
| mutate({ | |
| id: discussionId.toString(), | |
| body: { | |
| startDateTime: formatDateToDateTimeString(startDateTime), | |
| endDateTime: formatDateToDateTimeString(endDateTime), | |
| }, | |
| }); | |
| queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); | |
| queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); | |
| }} | |
| <Button | |
| onClick={() => { | |
| mutate({ | |
| id: discussionId.toString(), | |
| body: { | |
| startDateTime: formatDateToDateTimeString(startDateTime), | |
| endDateTime: formatDateToDateTimeString(endDateTime) | |
| } | |
| }, { | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); | |
| queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); | |
| // Add success toast/notification here | |
| }, | |
| onError: (error) => { | |
| console.error('Failed to confirm schedule:', error); | |
| // Add error toast/notification here | |
| } | |
| }); | |
| }} | |
| size='lg' | |
| > | |
| {/* Button content here */} | |
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance loading and error states.
The current loading and error states lack proper UI components and error information.
Apply this diff to improve the user experience:
- if (isPending) return <div>Pending...</div>;
- if (!data) return <div>data is undefined or null</div>;
+ if (isPending) return <LoadingSpinner />; // Add a proper loading component
+ if (!data) {
+ return (
+ <ErrorState
+ message="Failed to load schedule details"
+ action={<Button onClick={() => window.location.reload()}>Retry</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.
| if (isPending) return <div>Pending...</div>; | |
| if (!data) return <div>data is undefined or null</div>; | |
| if (isPending) return <LoadingSpinner />; // Add a proper loading component | |
| if (!data) { | |
| return ( | |
| <ErrorState | |
| message="Failed to load schedule details" | |
| action={<Button onClick={() => window.location.reload()}>Retry</Button>} | |
| /> | |
| ); | |
| } |
hamo-o
left a comment
hamo-o
left a comment
There was a problem hiding this comment.
조율페이지 수정이 급선무인 것 같습니닷.......!
frontend/src/features/discussion/ui/DiscussionTitle/DiscussionContext.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/utils/date/format.ts (1)
105-106: Add JSDoc documentation for consistency.For consistency with other functions in this file, add JSDoc documentation to describe the function's purpose and parameters.
+/** + * 날짜 객체를 요일 문자열로 변환합니다. + * @param date - 날짜 객체 + * @returns 요일 문자열 (예: '월', '화', etc.) + */ export const getDowString = (date: Date): string => date.toLocaleString('ko-KR', { weekday: 'short' });frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
76-85: Consider improving maintainability with constants and types.The component logic is correct, but could be enhanced for better maintainability.
+const NO_ADJUSTMENTS_NEEDED = 0; +type ChipColorType = 'red' | 'blue'; const AvailableChip = ({ adjustCount }: { adjustCount: number }) => ( <Chip - color={adjustCount > 0 ? 'red' : 'blue'} + color={adjustCount > NO_ADJUSTMENTS_NEEDED ? 'red' : 'blue' as ChipColorType} radius='max' size='md' style='weak' > - {adjustCount > 0 ? `조율 필요 ${adjustCount}` : '모두 가능'} + {adjustCount > NO_ADJUSTMENTS_NEEDED ? `조율 필요 ${adjustCount}` : '모두 가능'} </Chip> );frontend/src/features/discussion/ui/DiscussionCard/index.tsx (1)
19-29: Improve state object formatting and safety.The state object could be formatted more cleanly and safely.
Consider this refactoring:
- <Link - params={{ id: id }} - state={{ candidate: { - adjustCount: discussion.usersForAdjust.length, - startDateTime: discussion.startDateTime, - endDateTime: discussion.endDateTime, - selectedParticipantIds: memberContext?.formState.checkedList ?? [] - , - } }} - to={'/discussion/candidate/$id'} - > + <Link + params={{ id }} + to="/discussion/candidate/$id" + state={{ + candidate: { + adjustCount: discussion.usersForAdjust.length, + startDateTime: discussion.startDateTime, + endDateTime: discussion.endDateTime, + selectedParticipantIds: memberContext?.formState?.checkedList ?? [], + }, + }} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e06a14f77c47cd0ea08d18c00e19c8c3ed5ab86b and 8b1604f60aeec0d899ecab343eb6543f05098c7e.
📒 Files selected for processing (3)
frontend/src/features/discussion/ui/DiscussionCard/index.tsx(2 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/utils/date/format.ts(1 hunks)
🔇 Additional comments (7)
frontend/src/utils/date/format.ts (1)
105-106: LGTM! Clean and efficient implementation.The function is well-implemented:
- Uses the correct locale for Korean weekday names
- Follows functional programming principles
- Is properly typed
frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (3)
1-15: LGTM! Well-organized imports and type definitions.The imports are properly structured and include all necessary components, types, and utility functions needed for the component's functionality.
25-35: Improve key prop implementation.Using
JSON.stringify(candidate)in the key prop is not recommended as it's computationally expensive and could cause issues with circular references.- key={`${JSON.stringify(candidate)}-${idx}`} + key={candidate.id ?? idx}
39-74: LGTM! Well-structured component with proper date handling.The component effectively:
- Uses date utility functions for consistent formatting
- Properly passes candidate state through the Link component
- Correctly derives participant IDs from the participants prop
frontend/src/features/discussion/ui/DiscussionCard/index.tsx (3)
1-3: LGTM! Imports align with routing requirements.The imports from
@tanstack/react-routerand the member context are appropriate for the component's routing and state management needs.
30-34: Avoid type assertion for rank prop.The type assertion
rank as numbercould be unsafe. Consider handling the case where rank is undefined.
15-17:Details
❓ Verification inconclusive
Verify route context for useParams.
The
useParamshook is configured to expect a specific route pattern/_main/discussion/$id. Ensure this component is only rendered within this route context to avoid runtime errors.Run this script to check where this component is used:
🏁 Script executed:
#!/bin/bash # Search for DiscussionCard usage ast-grep --pattern 'import { DiscussionCard } from "$_" $$$ <DiscussionCard $_>'Length of output: 82
Ensure DiscussionCard is used only in the intended route context.
The component relies onuseParamswith the route pattern/_main/discussion/$id, so it will only work correctly when rendered within that specific route. As the initial search for its usage via ast-grep did not yield clear results, please manually verify all instances whereDiscussionCardis imported or rendered to ensure they adhere to this constraint. This helps prevent potential runtime errors when the component is rendered in an unexpected context.
There was a problem hiding this comment.
p2;
Link로 감싸지 않고, as 프롭으로 넘겨주면 스타일링이 유지될 것 같아요.
There was a problem hiding this comment.
왠진 모르겠지만 as={Link} 적용하니 state는 넘어가는데 param 전달이 안됩니다..ㅜ
+ to=route 타입 추론이 안됩니다
There was a problem hiding this comment.
p3;
every와 some 고차함수에 대해 알아보시면 좋을 것 같습니다.
+ let 멈춰 ㅜ~~
There was a problem hiding this comment.
p2; 홈에서 들어갈 때는 괜찮은데 일정 조율에서 들어가면 선택된 유저가 제대로 표시가 안되네요
+ 딤처리 레이아웃 밀립니다아
There was a problem hiding this comment.
다음 pr에서 수정하겠습니다! 관련해서 고친 내용이 있어서요
dioo1461
force-pushed
the
feature/fe/candidate-detail-api
branch
from
396fc1c to
eb3727b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (1)
78-85:⚠️ Potential issueAvoid using array index as key prop.
Using array indices as keys can lead to issues with component rerendering and state management, especially if the list items can be reordered or filtered.
- key={index} + key={schedule.discussionId}frontend/src/features/candidate-schedule/ui/Header.tsx (3)
28-32:⚠️ Potential issueFix inconsistent prop names in HeaderTextInfo usage.
The props have been renamed to
startDateTimeandendDateTime, but they are passed asstartTimeandendTimeto HeaderTextInfo.Apply this diff to maintain consistency:
<HeaderTextInfo adjustCount={adjustCount} - endTime={endDateTime} - startTime={startDateTime} + endDateTime={endDateTime} + startDateTime={startDateTime} />
34-34: 🛠️ Refactor suggestionStandardize date management across the codebase.
The TODO comment indicates inconsistency in date handling. Consider adopting a consistent approach:
- Use
Dateobjects internally- Convert to strings only at API boundaries
- Consider using a date management library like
date-fnsorluxon
36-46: 🛠️ Refactor suggestionAdd error handling and user feedback for mutation.
The mutation lacks error handling and user feedback. Users won't know if the schedule confirmation succeeded or failed.
Apply this diff to add proper error handling and feedback:
<Button onClick={() => { mutate({ id: discussionId.toString(), body: { startDateTime: formatDateToDateTimeString(startDateTime), endDateTime: formatDateToDateTimeString(endDateTime), }, + }, { + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); + queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); + // Add success toast/notification here + }, + onError: (error) => { + console.error('Failed to confirm schedule:', error); + // Add error toast/notification here + }, }); - queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); - queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); }} size='lg' >frontend/src/features/candidate-schedule/ui/index.tsx (1)
29-30: 🛠️ Refactor suggestionEnhance loading and error states.
The current loading and error states lack proper UI components and error information.
Apply this diff to improve the user experience:
- if (isPending) return <div>Pending...</div>; - if (!data) return <div>data is undefined or null</div>; + if (isPending) return <LoadingSpinner />; // Add a proper loading component + if (!data) { + return ( + <ErrorState + message="Failed to load schedule details" + action={<Button onClick={() => window.location.reload()}>Retry</Button>} + /> + ); + }
🧹 Nitpick comments (10)
frontend/src/features/discussion/ui/DiscussionCard/index.tsx (1)
20-31: Clean up state object and improve type safety.
- Remove the trailing comma after the last property.
- Consider using a more type-safe fallback for
selectedParticipantIds.state={{ candidate: { adjustCount: discussion.usersForAdjust.length, startDateTime: discussion.startDateTime, endDateTime: discussion.endDateTime, - selectedParticipantIds: memberContext?.formState.checkedList ?? [] - , + selectedParticipantIds: memberContext?.formState.checkedList ?? [], } }}frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (4)
18-22: Consider using PropTypes or TypeScript interface for props validation.While TypeScript inline types are used, consider extracting the props interface for better reusability and documentation:
interface RecommendedSchedulesProps { candidates: DiscussionCalendarResponse['events']; discussion: DiscussionResponse; participants: UserDTO[]; } const RecommendedSchedules = ({ candidates, discussion, participants }: RecommendedSchedulesProps) => ...
57-62: Consider passing minimal state in navigation.The state object contains derived data that could be computed in the target component. Consider passing only the essential data:
- state={{ candidate: { - adjustCount: candidate.usersForAdjust.length, - startDateTime: startDTStr, - endDateTime: endDTStr, - selectedParticipantIds: participants.map(v => v.id), - } }} + state={{ candidateId: candidate.id }}
49-52: Memoize date calculations for better performance.The date calculations are performed on every render. Consider using useMemo to optimize performance:
const dateInfo = useMemo(() => { const [startDT, endDT] = [new Date(startDTStr), new Date(endDTStr)]; const [startTime, endTime] = [getTimeParts(startDT), getTimeParts(endDT)]; const { month, day } = getYearMonthDay(startDT); const dow = getDowString(startDT); return { startDT, endDT, startTime, endTime, month, day, dow }; }, [startDTStr, endDTStr]);
76-84: Extract magic number and add prop types.Consider these improvements for better maintainability:
const NO_ADJUSTMENT_NEEDED = 0; interface AvailableChipProps { adjustCount: number; } const AvailableChip = ({ adjustCount }: AvailableChipProps) => ( <Chip color={adjustCount > NO_ADJUSTMENT_NEEDED ? 'red' : 'blue'} radius='max' size='md' style='weak' > {adjustCount > NO_ADJUSTMENT_NEEDED ? `조율 필요 ${adjustCount}` : '모두 가능'} </Chip> );frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (3)
26-33: Consider adding error handling for edge cases.While the useEffect hook correctly handles the case when the selected discussion is not found, it might benefit from additional error handling:
- Handle the case when data is empty but selectedId exists
- Consider debouncing the selection change to prevent rapid re-renders
useEffect(() => { - if (data && data.ongoingDiscussions.length > 0) { + if (!data || data.ongoingDiscussions.length === 0) { + // Clear selection when no data is available + onSelect(0); + return; + } + const exists = data.ongoingDiscussions.some( (schedule) => schedule.discussionId === selectedId, ); if (!exists) onSelect(data.ongoingDiscussions[0].discussionId); - } }, [data, selectedId, onSelect]);
35-61: Consider adding loading state and accessibility improvements.The loading and error states could be enhanced for better user experience:
- Add aria-live region for loading state
- Provide more descriptive error messages
- Add aria-busy attribute during loading
- if (isPending) return <div>pending...</div>; - if (!data || data.ongoingDiscussions.length === 0) return <div>no data available</div>; + if (isPending) return <div aria-live="polite" aria-busy="true">Loading ongoing schedules...</div>; + if (!data || data.ongoingDiscussions.length === 0) { + return ( + <div aria-live="polite" role="status"> + No ongoing schedules available at this time. + </div> + ); + }
89-90: Consider adding explicit type for static Item property.While the static Item property works, it would benefit from explicit typing for better maintainability.
+ type OngoingScheduleListComponent = typeof OngoingScheduleList & { + Item: typeof OngoingScheduleListItem; + }; - OngoingScheduleList.Item = OngoingScheduleListItem; + (OngoingScheduleList as OngoingScheduleListComponent).Item = OngoingScheduleListItem; - export default OngoingScheduleList; + export default OngoingScheduleList as OngoingScheduleListComponent;frontend/src/pages/UpcomingSchedulePage/index.tsx (2)
5-6: Consolidate the split import path.The import path for
UpcomingScheduleListis unnecessarily split across two lines. Consider consolidating it for better readability.-import UpcomingScheduleList from - '@/features/shared-schedule/ui/UpcomingSchedules/UpcomingScheduleList'; +import UpcomingScheduleList from '@/features/shared-schedule/ui/UpcomingSchedules/UpcomingScheduleList';
28-30: Extract magic number and add empty state handling.The code uses a magic number (3) for slicing the schedules and doesn't handle empty states for the carousel and list views.
Consider these improvements:
+const CAROUSEL_LIMIT = 3; + const UpcomingSchedulePage = () => { // ... existing code ... - <UpcomingCarousel schedules={schedules.slice(0, 3)} /> + {schedules.length > 0 ? ( + <> + <UpcomingCarousel schedules={schedules.slice(0, CAROUSEL_LIMIT)} /> + {schedules.length > CAROUSEL_LIMIT && ( + <UpcomingScheduleList schedules={schedules.slice(CAROUSEL_LIMIT)} /> + )} + </> + ) : ( + <Text typo="body1">표시할 일정이 없습니다.</Text> + )} - <UpcomingScheduleList schedules={schedules.slice(3)} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 396fc1c14ca147551d822eeb1da974afb3a866ee and eb3727b.
📒 Files selected for processing (38)
frontend/src/features/candidate-schedule/api/index.ts(1 hunks)frontend/src/features/candidate-schedule/api/keys.ts(1 hunks)frontend/src/features/candidate-schedule/api/queries.ts(1 hunks)frontend/src/features/candidate-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/candidate-schedule/model/index.ts(1 hunks)frontend/src/features/candidate-schedule/ui/Header.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx(2 hunks)frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts(4 hunks)frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx(1 hunks)frontend/src/features/candidate-schedule/ui/index.tsx(1 hunks)frontend/src/features/discussion/model/index.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionCard/card.css.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionCard/index.tsx(2 hunks)frontend/src/features/shared-schedule/api/keys.ts(1 hunks)frontend/src/features/shared-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/shared-schedule/model/index.ts(0 hunks)frontend/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx(4 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(3 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx(2 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx(3 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx(1 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingScheduleList.tsx(2 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts(0 hunks)frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx(2 hunks)frontend/src/main.tsx(1 hunks)frontend/src/pages/CandidateSchedulePage/index.tsx(1 hunks)frontend/src/pages/DiscussionPage/MemberContext.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/$id.lazy.tsx(1 hunks)frontend/src/routes/_main/discussion/candidate/index.lazy.tsx(0 hunks)frontend/src/utils/date/format.ts(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/features/shared-schedule/model/index.ts
- frontend/src/routes/_main/discussion/candidate/index.lazy.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.css.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- frontend/src/features/discussion/ui/DiscussionCard/card.css.ts
- frontend/src/pages/DiscussionPage/MemberContext.ts
- frontend/src/pages/UpcomingSchedulePage/index.css.ts
- frontend/src/utils/date/format.ts
- frontend/src/features/candidate-schedule/api/queries.ts
- frontend/src/pages/HomePage/index.tsx
- frontend/src/features/candidate-schedule/ui/TImelineCanvas/TimelineBlocks.tsx
- frontend/src/features/candidate-schedule/api/queryOptions.ts
- frontend/src/features/discussion/ui/DiscussionCalendar/CalendarTable.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/index.tsx
- frontend/src/main.tsx
- frontend/src/features/candidate-schedule/api/keys.ts
- frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingScheduleList.tsx
- frontend/src/features/discussion/model/index.ts
- frontend/src/features/candidate-schedule/api/index.ts
- frontend/src/features/candidate-schedule/ui/TimelineContent/ParticipantList.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/index.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/recommendedSchedules.css.ts
- frontend/src/features/shared-schedule/api/keys.ts
- frontend/src/features/shared-schedule/ui/UpcomingSchedules/UpcomingCarousel.tsx
- frontend/src/features/candidate-schedule/ui/TImelineCanvas/index.css.ts
- frontend/src/features/discussion/ui/DiscussionCalendar/CalendarDate.tsx
- frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx
🔇 Additional comments (19)
frontend/src/features/discussion/ui/DiscussionCard/index.tsx (3)
1-14: LGTM!The imports and type definitions are well-structured and align with the component's enhanced routing and context requirements.
16-19: LGTM! Changes align with previous feedback.The component now correctly uses
useParamsanduseMemberContextas suggested in the past review comments.
32-36: Avoid type assertion for rank prop.The type assertion
rank as numbercould be unsafe. Consider handling the case where rank is undefined.- <DiscussionLarge discussion={discussion} rank={rank as number} /> + <DiscussionLarge + discussion={discussion} + rank={rank ?? 0} // or handle undefined case appropriately + />frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (1)
31-31: Improve key prop implementation.Using
JSON.stringify(candidate)in the key prop is computationally expensive and could cause issues with circular references.- key={`${JSON.stringify(candidate)}-${idx}`} + key={candidate.id ?? idx}frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (2)
2-2: LGTM! Clean and clear changes.The addition of useEffect import and the renaming of the constant to PAGE_SIZE improves code clarity and follows better naming conventions.
Also applies to: 14-14
25-25: LGTM! Clean query setup.The query setup is well-structured and properly uses the renamed PAGE_SIZE constant.
frontend/src/features/shared-schedule/api/queryOptions.ts (3)
4-4: LGTM! Good improvement in import organization.Breaking down the query key imports into individual named imports improves code maintainability and makes dependencies more explicit.
8-8: LGTM! Clean and consistent query key usage.Direct usage of the imported
upcomingQueryKeymaintains functionality while improving code clarity.
12-12:Details
❓ Verification inconclusive
LGTM! Verify consistent query key usage across components.
The changes to both ongoing and finished schedule query keys follow a consistent pattern using the
.detailmethod. This improves code organization while maintaining the same functionality.Let's verify that this pattern is consistently used across components:
Also applies to: 16-16
🏁 Script executed:
#!/bin/bash # Search for any inconsistent usage of these query keys rg -A 2 "queryKey.*(?:ongoing|finished|upcoming)(?!Key)" frontend/srcLength of output: 360
Please run the above script to confirm that there are no inconsistent usages of the query keys across the codebase.
LGTM! Consistent query key usage confirmed
Both the ongoing and finished schedule query keys now consistently use the.detailmethod, which improves code organization without affecting functionality. This pattern, as reflected on line 12 (and similarly on line 16), appears to be uniformly applied.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run the search using PCRE2 to support negative lookahead and verify query key consistency rg --pcre2 -A 2 "queryKey.*(?:ongoing|finished|upcoming)(?!Key)" frontend/srcLength of output: 1511
LGTM for the shared-schedule changes!
The updates infrontend/src/features/shared-schedule/api/queryOptions.tsthat switch to using.detailfor both ongoing (ongoingQueryKey.detail(page, size, attendtype)) and finished (finishedQueryKey.detail(page, size, year)) schedules improve clarity and maintain API compatibility.However, while verifying the codebase, I noticed that in the candidate-schedule component (
frontend/src/features/candidate-schedule/ui/Header.tsx), the query keys are used slightly differently (e.g.,upcomingQueryKeyis passed directly andongoingQueryKey.allis used for invalidating queries). Please confirm that these differences are intentional and align with the caching strategies for candidate schedules.frontend/src/pages/CandidateSchedulePage/index.tsx (1)
14-14: Implement error handling for missing candidate data.The TODO comment indicates missing error handling. This should be addressed to prevent potential runtime errors.
frontend/src/features/shared-schedule/model/upcomingSchedules.ts (2)
5-9: LGTM! Well-structured schema definition.The
SharedEventDtoSchemais well-defined with appropriate types and date coercion.
11-17: LGTM! Clear and comprehensive schema structure.The
UpcomingScheduleSchemaeffectively combines the shared event data with schedule-specific fields.frontend/src/features/candidate-schedule/model/index.ts (2)
19-26: LGTM! Well-structured participant schema.The
ParticipantSchemais well-defined with appropriate types and the newrequirementOfAdjustmentfield.
5-9:Details
✅ Verification successful
Verify API alignment for status values.
The status values have been updated to uppercase. Please verify that these values match the API contract.
🌐 Web query:
What is the standard convention for enum values in REST APIs?💡 Result:
The standard convention for enum values in REST APIs involves a combination of naming styles and best practices to ensure clarity, consistency, and compatibility:
Enum type naming
- UpperCamelCase for enum types (e.g.,
UserRole,OrderStatus) [8][9]- Should use singular nouns (e.g.,
Colorrather thanColors) [8]Enum value naming
CAPITALIZED_SNAKE_CASE (uppercase with underscores) is most common in industry standards:
enum UserRole { ADMINISTRATOR = "ADMINISTRATOR", POWER_USER = "POWER_USER", GUEST = "GUEST" }[1][8][10]
Alternative conventions exist but are less common:
- kebab-case (
admin-user)- PascalCase (
Administrator)- lowercase (
admin) [9]Implementation best practices
Default value handling:
- Include an
UNSPECIFIEDvalue as first entry (0-indexed) when using numeric enums:[8]enum SortOrder { SORT_ORDER_UNSPECIFIED = 0; ASCENDING = 1; DESCENDING = 2; }String vs numeric:
- Prefer string enums in REST APIs for better readability in JSON:
[1][6]{"status": "SHIPPED"}Documentation:
- Explicitly list allowed values in OpenAPI specifications:
[4][5]status: type: string enum: [CREATED, PROCESSING, SHIPPED]Versioning considerations:
- Treat enum additions as backward-compatible changes
- Avoid removing or modifying existing enum values [6]
Convention Type Recommendation Example Enum Type Name UpperCamelCase ShippingMethodEnum Value Format CAPITALIZED_SNAKE_CASE EXPRESS_DELIVERYDefault Value *_UNSPECIFIEDSTATUS_UNSPECIFIEDThese conventions balance machine readability with human comprehension while maintaining compatibility with API documentation tools and client SDKs[1][4][8]. Teams should choose one style consistently across their API surface area[10].
Citations:
- 1: https://appwrite.io/blog/post/enums-api-design
- 2: https://www.youtube.com/watch?v=i56lz1wAh6M
- 3: https://opensource.zalando.com/restful-api-guidelines/
- 4: https://swagger.io/docs/specification/v3_0/data-models/enums/
- 5: https://dev.to/timothymcgrath/enums-apis-15n4
- 6: https://tyk.io/blog/api-design-guidance-enums/
- 7: https://www.outsystems.com/forums/discussion/101021/rest-api-integration-enum/
- 8: https://cloud.google.com/apis/design/naming_convention
- 9: zalando/restful-api-guidelines#530
- 10: https://help.mulesoft.com/s/question/0D56R00008jxyteSAA/enum-values-naming-convention
API Enum Values Verified: Changes Align with Standard Conventions
The updated schema now uses the uppercase values ('ADJUSTABLE', 'FIXED', and 'OUT_OF_RANGE'), which follow the widely accepted CAPITALIZED_SNAKE_CASE convention for enum values in REST APIs. This naming style is consistent with industry best practices and the API contract.
No further changes are needed.
frontend/src/features/shared-schedule/ui/UpcomingSchedules/ScheduleCard.tsx (1)
25-31: LGTM! Proper property access from sharedEventDto.The date-time properties are correctly accessed from the
sharedEventDtoobject, aligning with the schema changes.frontend/src/features/candidate-schedule/ui/Header.tsx (2)
11-16: LGTM! Props interface is well-defined.The
HeaderPropsinterface clearly defines the required props with appropriate types.
61-77: LGTM! Improved text rendering logic.The conditional text rendering based on
needsAdjustis clear and maintainable.frontend/src/features/candidate-schedule/ui/index.tsx (2)
49-68: LGTM! Well-implemented navigation logic.The
TopBarcomponent correctly handles navigation with proper checks usinguseCanGoBack.
70-74: LGTM! Props interface is well-defined.The
ContentPropsinterface correctly extendsCandidateDetailResponseand includes additional required props.
| const { data, isPending } = useUpcomingQuery(); | ||
| if (isPending) return <div>Pending...</div>; | ||
| if (!data) return <div>data is undefined or null</div>; | ||
| const schedules = data.data; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and loading states.
The current implementation has several areas for improvement:
- Loading state shows a generic message
- Error state exposes implementation details
- No error boundary for runtime errors
- No type safety for the data structure
Consider implementing these improvements:
+import { ErrorBoundary } from '@/components/ErrorBoundary';
+import { LoadingSpinner } from '@/components/LoadingSpinner';
+import type { UpcomingScheduleResponse } from '@/features/shared-schedule/types';
const UpcomingSchedulePage = () => {
- const { data, isPending } = useUpcomingQuery();
+ const { data, isPending, error } = useUpcomingQuery<UpcomingScheduleResponse>();
+
+ if (error) return <div>일정을 불러오는 중 오류가 발생했습니다.</div>;
+ if (isPending) return <LoadingSpinner />;
+ if (!data?.data) return <div>일정이 없습니다.</div>;
- if (isPending) return <div>Pending...</div>;
- if (!data) return <div>data is undefined or null</div>;
const schedules = data.data;📝 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(); | |
| if (isPending) return <div>Pending...</div>; | |
| if (!data) return <div>data is undefined or null</div>; | |
| const schedules = data.data; | |
| import { ErrorBoundary } from '@/components/ErrorBoundary'; | |
| import { LoadingSpinner } from '@/components/LoadingSpinner'; | |
| import type { UpcomingScheduleResponse } from '@/features/shared-schedule/types'; | |
| // ... other necessary imports | |
| const UpcomingSchedulePage = () => { | |
| const { data, isPending, error } = useUpcomingQuery<UpcomingScheduleResponse>(); | |
| if (error) return <div>일정을 불러오는 중 오류가 발생했습니다.</div>; | |
| if (isPending) return <LoadingSpinner />; | |
| if (!data?.data) return <div>일정이 없습니다.</div>; | |
| const schedules = data.data; | |
| // ... rest of the component logic and JSX | |
| }; | |
| export default UpcomingSchedulePage; |
| endDateTime: string; | ||
| selectedParticipantIds: number[]; | ||
| }) => { | ||
| const { id } = useParams({ from: '/_main/discussion/candidate/$id' }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type guard for route parameters.
The id parameter from useParams should be validated to ensure it exists and is a valid number.
- const { id } = useParams({ from: '/_main/discussion/candidate/$id' });
+ const { id } = useParams({ from: '/_main/discussion/candidate/$id' });
+ if (!id || isNaN(Number(id))) {
+ return <div>Invalid discussion ID</div>;
+ }📝 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 { id } = useParams({ from: '/_main/discussion/candidate/$id' }); | |
| const { id } = useParams({ from: '/_main/discussion/candidate/$id' }); | |
| if (!id || isNaN(Number(id))) { | |
| return <div>Invalid discussion ID</div>; | |
| } |
| const NavigateButton = ({ latest }: { | ||
| latest: boolean; | ||
| schedule: UpcomingSchedule; | ||
| }) => ( | ||
| <button className={chevronButtonStyle({ latest })}> | ||
| <ChevronRight | ||
| clickable | ||
| fill={vars.color.Ref.Netural[800]} | ||
| width={28} | ||
| /> | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused schedule prop from NavigateButton.
The schedule prop is declared but never used in the component.
-const NavigateButton = ({ latest }: {
- latest: boolean;
- schedule: UpcomingSchedule;
-}) => (
+const NavigateButton = ({ latest }: {
+ latest: boolean;
+}) => (
<button className={chevronButtonStyle({ latest })}>
<ChevronRight
clickable
fill={vars.color.Ref.Netural[800]}
width={28}
/>
</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.
| const NavigateButton = ({ latest }: { | |
| latest: boolean; | |
| schedule: UpcomingSchedule; | |
| }) => ( | |
| <button className={chevronButtonStyle({ latest })}> | |
| <ChevronRight | |
| clickable | |
| fill={vars.color.Ref.Netural[800]} | |
| width={28} | |
| /> | |
| </button> | |
| ); | |
| const NavigateButton = ({ latest }: { | |
| latest: boolean; | |
| }) => ( | |
| <button className={chevronButtonStyle({ latest })}> | |
| <ChevronRight | |
| clickable | |
| fill={vars.color.Ref.Netural[800]} | |
| width={28} | |
| /> | |
| </button> | |
| ); |
| startDateTime: string; | ||
| endDateTime: string; |
There was a problem hiding this comment.
Align type definitions with date management strategy.
The startDateTime and endDateTime props are defined as strings, but they're used as Date objects in the Header component.
Apply this diff to maintain consistency:
interface CandidateScheduleDetailProps {
adjustCount: number;
discussionId: number;
- startDateTime: string;
- endDateTime: string;
+ startDateTime: Date;
+ endDateTime: Date;
selectedParticipantIds: number[];
}📝 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.
| startDateTime: string; | |
| endDateTime: string; | |
| interface CandidateScheduleDetailProps { | |
| adjustCount: number; | |
| discussionId: number; | |
| startDateTime: Date; | |
| endDateTime: Date; | |
| selectedParticipantIds: number[]; | |
| } |
hamo-o
left a comment
hamo-o
left a comment
There was a problem hiding this comment.
찐막: - 1번째 아이템들을 prefetch 해오지 않고 -1로 되네요?! 그래서 500이 뜹니당
요것도 인지하고 있습니다,, 버그투성이네요 ㅜㅜ |
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
New Features
CalendarDatecomponent for displaying discussions by date.RecommendedSchedulesandUpcomingCarousel.Refactor
Style
Chore