Skip to content

Conversation

@dioo1461
Copy link
Contributor

@dioo1461 dioo1461 commented Feb 21, 2025

#️⃣ 연관된 이슈>

📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

  • 마감기한을 잘못 계산하던 문제 수정
  • 다가오는일정 세부 타임라인 화면에서 실제보다 달이 1 적게 출력되던 문제 수정

🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

Summary by CodeRabbit

  • New Features
    • Introduced a new Deadline component for enhanced deadline display, showing time remaining or elapsed.
    • Added query invalidation logic to the DiscussionConfirmButton for improved data refresh upon discussion confirmation.
  • Bug Fixes
    • Updated the schedule detail header to correctly display the month by adjusting the displayed value.
  • Refactor
    • Simplified the Header component in the CandidateSchedulePage by removing direct mutation logic and introducing a dedicated button component.
    • Streamlined date handling in the CandidateSchedulePage by passing string values directly to the Header component.
  • New Utilities
    • Added functions for calculating time left from milliseconds and formatting time to deadline strings.
  • Chores
    • Changed visibility of the HOUR_IN_MILLISECONDS constant to be accessible across modules.

@dioo1461 dioo1461 requested a review from hamo-o as a code owner February 21, 2025 04:00
@coderabbitai
Copy link

coderabbitai bot commented Feb 21, 2025

Walkthrough

This update refactors how deadline information and date display are handled in the frontend. In the schedule details component, the previous getDday function is removed and replaced with a new Deadline component that computes and conditionally displays the deadline message. In the header component of the upcoming schedule details page, the month value is incremented by one before rendering. Additionally, new query invalidation logic is introduced in the discussion confirm button, and various date handling improvements are made throughout the components.

Changes

File(s) Change Summary
frontend/src/features/.../ScheduleDetails.tsx Removed getDday function and its usage in ScheduleInfo; introduced a new Deadline component to calculate and display deadline details; added formatTimeToDeadlineString function.
frontend/src/pages/.../Header.tsx Modified the month display logic by incrementing the month value; added a TODO comment for unifying the date formatting method.
frontend/src/features/discussion/ui/DiscussionConfirmButton/index.tsx Added query invalidation logic using useQueryClient to refetch relevant queries upon discussion confirmation.
frontend/src/pages/.../CandidateSchedulePage/Header.tsx Removed discussionId from HeaderProps; changed startDateTime and endDateTime types from Date to string; simplified implementation by introducing DiscussionConfirmButton.
frontend/src/pages/.../CandidateSchedulePage/index.tsx Removed local date parsing for startDateTime and endDateTime; passed original string values directly to the Header component.
frontend/src/utils/date/date.ts Added getTimeLeftInfoFromMilliseconds function to calculate days, hours, and minutes from milliseconds.
frontend/src/utils/date/format.ts Introduced formatTimeToDeadlineString function for formatting deadline time remaining.
frontend/src/utils/date/time.ts Changed HOUR_IN_MILLISECONDS from private to exported constant for broader accessibility.
frontend/src/features/.../OngoingScheduleListItem.tsx Introduced a new Deadline component for displaying deadline messages; removed commented-out code for update notifications.
frontend/src/routes/.../home/index.tsx Added a blank line for improved readability; no functional changes.

Possibly related PRs

Suggested labels

🛠️ BE

Suggested reviewers

  • kwon204
  • hamo-o

Poem

Oh, I hop through lines of code so bright,
Deadlines now tick with a new insight.
The Deadline component leads the way,
While headers shine with a fresher display.
A rabbit's cheer in every bug we fight!
🐇 Happy code hops into the night!

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@dioo1461 dioo1461 changed the title [FE-Fix] 자잘한 문제 수정 [FE-Fix] 마감기한을 잘못 계산하던 문제, 다가오는일정 세부 타임라인 화면에서 실제보다 달이 1 적게 출력되던 문제 수정 Feb 21, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (2)

100-101: Consider implementing the TODO items now.

These TODOs are directly related to the deadline calculation bug fix:

  1. Moving calculation logic to a utility would improve testability
  2. Special handling for D-day could improve user experience

Consider implementing a utility like this:

// utils/deadline.ts
export const calculateDeadline = (timeLeft: number) => {
  const daysLeft = Math.floor(timeLeft / (1000 * 60 * 60 * 24));
  return {
    daysLeft,
    isExpired: daysLeft < 0,
    isDday: daysLeft === 0
  };
};

102-109: Improve deadline message formatting and edge cases.

The current implementation has some potential improvements:

  1. The negative days message could be confusing
  2. No special handling for D-day (when daysLeft = 0)
  3. No handling for edge cases (e.g., very large timeLeft values)

Consider this enhanced implementation:

const Deadline = ({ timeLeft }: { timeLeft: number }) => {
  const daysLeft = Math.floor(timeLeft / (1000 * 60 * 60 * 24));
  
  if (daysLeft === 0) {
    return <Text color={vars.color.Ref.Red[500]} typo='b3M'>오늘이 마감일이에요!</Text>;
  }
  
  if (daysLeft > 0) {
    return <Text color={vars.color.Ref.Red[500]} typo='b3M'>{`마감까지 ${daysLeft}일`}</Text>;
  }
  
  const daysOverdue = Math.abs(daysLeft);
  return <Text color={vars.color.Ref.Red[500]} typo='b3M'>{`마감일이 ${daysOverdue}일 지났어요`}</Text>;
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08c49bf and 9ee56d8d833de1329399cf935804727786ebc594.

📒 Files selected for processing (2)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (3 hunks)
  • frontend/src/pages/UpcomingScheduleDetailPage/Header.tsx (2 hunks)

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider fixing the root cause of month offset.

While adding +1 fixes the immediate issue, it might be better to address this at the source. The getDateParts utility might be returning 0-based months (common in JavaScript Date).

Consider these alternatives:

  1. Fix the getDateParts utility to return 1-based months:
- const { month, day } = getDateParts(startDateTime);
- {`${month + 1}월 ${day}일`}
+ const { month, day } = getDateParts(startDateTime);
+ {`${month}월 ${day}일`}
  1. Or make the 0-based month handling explicit in the component:
- {`${month + 1}월 ${day}일`}
+ {`${getMonthDisplay(month)}월 ${day}일`}

Add a utility function:

const getMonthDisplay = (zeroBasedMonth: number) => zeroBasedMonth + 1;

Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Consider implementing the TODO comment sooner.

The TODO comment suggests consolidating format methods. Given that date formatting issues led to bugs, this consolidation could prevent similar issues in the future.

Let's check for other date formatting methods in the codebase:


🏁 Script executed:

#!/bin/bash
# Search for date formatting related functions
rg -l "format.*Date|Date.*format" --type ts --type tsx

Length of output: 84


I'll generate an updated shell script to search for date formatting functions by explicitly using glob patterns for both TS and TSX files. This avoids the file type error we encountered earlier. After running this, we can verify if there are date formatting functions elsewhere that could be consolidated with the eventual TODO in Header.tsx.

Once you run this script and share the output, we'll have better insight into whether other date formatting methods exist in the codebase, which will help refine the review comment further.


🏁 Script executed:

[run_scripts]
#!/bin/bash
rg -l "format.*Date|Date.*format" --glob "*.tsx" --glob "*.ts"

Length of output: 1804


Refactor date formatting methods for consistency

The current workaround—adding +1 to the month in Header.tsx—correctly fixes the immediate display bug but appears to mask an underlying date handling issue (likely related to 0-based months in the getDateParts logic). The lingering TODO comment suggests consolidating date format methods, and our search confirms that similar functionality exists across the codebase (e.g., in frontend/src/utils/date/format.ts and others). To prevent future bugs and improve maintainability, please prioritize refactoring these utilities into a unified solution.

  • Verify if the root cause (e.g., 0-based month handling) can be addressed in a central date extraction/formatting utility.
  • Consolidate the multiple date formatting methods into one robust implementation.
  • Ensure that the refactored solution works consistently across all affected components.

Copy link
Contributor

Choose a reason for hiding this comment

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

p2;
당일인 경우 시간을, 1시간 이하 남았을 때는 분을 표시해야 합니다.

image

@dioo1461
Copy link
Contributor Author

todo

  • 마감기한 출력 포맷 변경
  • 일정확정하기 버튼 공용으로 변경

@dioo1461 dioo1461 force-pushed the bugfix/fe/style-edit branch from 9ee56d8 to 7b28c57 Compare February 21, 2025 04:54
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (2)
frontend/src/features/discussion/ui/DiscussionConfirmButton/index.tsx (2)

21-29: 🛠️ Refactor suggestion

Add error handling and loading state.

The mutation callback lacks error handling and loading state management.

+ const [isLoading, setIsLoading] = useState(false);
  const handleClickConfirm = () => {
    if (!param.id) return;
+   setIsLoading(true);
    queryClient.invalidateQueries({ queryKey: upcomingQueryKey });
    queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all });
    mutate({
      id: param.id,
      body: { startDateTime, endDateTime },
+   }, {
+     onError: (error) => {
+       console.error('Failed to confirm discussion:', error);
+       // TODO: Add error toast notification
+     },
+     onSettled: () => setIsLoading(false),
    });
  };

31-33: 🛠️ Refactor suggestion

Add loading state to the button.

The button should reflect its loading state to prevent multiple clicks.

  return (
-   <Button onClick={handleClickConfirm}>일정 확정하기</Button>
+   <Button 
+     onClick={handleClickConfirm}
+     disabled={isLoading}
+   >
+     {isLoading ? '확정 중...' : '일정 확정하기'}
+   </Button>
  );
♻️ Duplicate comments (1)
frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)

103-115: ⚠️ Potential issue

Implement time display requirements for same-day deadlines.

Based on previous feedback, the deadline display needs to show:

  • Hours when the deadline is today
  • Minutes when less than 1 hour remains

Here's how to implement these requirements:

 const Deadline = ({ timeLeft }: { timeLeft: number }) => {
   const timeLeftInfo = getTimeLeftInfoFromMilliseconds(timeLeft);
   const isExpired = timeLeft < 0;
+  const isToday = timeLeftInfo.days === 0;
+  const isLessThanHour = isToday && timeLeftInfo.hours === 0;
+
   return (
     <Text color={vars.color.Ref.Red[500]} typo='b3M'>
       {isExpired ? 
         `마감일로부터 ${formatTimeToDeadlineString(timeLeftInfo)} 지났어요`
         : 
-        `마감까지 ${formatTimeToDeadlineString(timeLeftInfo)}`}
+        `마감까지 ${formatTimeToDeadlineString(timeLeftInfo, { forceHours: isToday, forceMinutes: isLessThanHour })}`}
     </Text>
   );
 };
🧹 Nitpick comments (3)
frontend/src/pages/CandidateSchedulePage/index.tsx (1)

29-30: LGTM! Consider adding type safety for date strings.

The change to pass raw date strings simplifies the date handling logic. However, consider using a more specific type than string to ensure date string format consistency.

interface CandidateProps {
  adjustCount: number;
-  startDateTime: string;
-  endDateTime: string;
+  startDateTime: `${number}-${number}-${number}T${number}:${number}:${number}`;
+  endDateTime: `${number}-${number}-${number}T${number}:${number}:${number}`;
  selectedParticipantIds?: number[];
}
frontend/src/pages/CandidateSchedulePage/Header.tsx (2)

23-24: Consider moving date parsing to a utility function.

The new Date() calls could fail silently with invalid date strings. Consider adding validation and error handling.

+ const parseDateSafely = (dateString: string): Date => {
+   const date = new Date(dateString);
+   if (isNaN(date.getTime())) {
+     throw new Error(`Invalid date string: ${dateString}`);
+   }
+   return date;
+ };

  <HeaderTextInfo
    adjustCount={adjustCount}
-   endTime={new Date(endDateTime)}
-   startTime={new Date(startDateTime)}
+   endTime={parseDateSafely(endDateTime)}
+   startTime={parseDateSafely(startDateTime)}
  />

33-37: Add type safety for HeaderTextInfo props.

Consider using a more specific type for the date-related props.

+ type DateTimeProps = {
+   adjustCount: number;
+   startTime: Date;
+   endTime: Date;
+ };

- const HeaderTextInfo = ({ adjustCount, startTime, endTime }: {
-   adjustCount: number;
-   startTime: Date;
-   endTime: Date;
- }) => {
+ const HeaderTextInfo = ({ adjustCount, startTime, endTime }: DateTimeProps) => {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ee56d8d833de1329399cf935804727786ebc594 and 7b28c575eaace6d60d8b51c3f6c1d536f371a945.

📒 Files selected for processing (8)
  • frontend/src/features/discussion/ui/DiscussionConfirmButton/index.tsx (2 hunks)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (3 hunks)
  • frontend/src/pages/CandidateSchedulePage/Header.tsx (1 hunks)
  • frontend/src/pages/CandidateSchedulePage/index.tsx (1 hunks)
  • frontend/src/pages/UpcomingScheduleDetailPage/Header.tsx (2 hunks)
  • frontend/src/utils/date/date.ts (2 hunks)
  • frontend/src/utils/date/format.ts (1 hunks)
  • frontend/src/utils/date/time.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/pages/UpcomingScheduleDetailPage/Header.tsx
🔇 Additional comments (3)
frontend/src/utils/date/time.ts (1)

2-2: LGTM! Export of HOUR_IN_MILLISECONDS enhances reusability.

The constant is now properly exported for use in time calculations across the codebase.

frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)

14-18: LGTM! Clean import organization.

The imports are well-organized and properly grouped.

frontend/src/utils/date/date.ts (1)

251-260: LGTM! Clean implementation of time unit extraction.

The function correctly breaks down milliseconds into days, hours, and minutes using proper mathematical operations.

Comment on lines +109 to +117
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance formatTimeToDeadlineString to support forced time unit display.

To meet the requirements of showing hours for same-day deadlines and minutes for less than an hour remaining, the function needs additional parameters.

 export const formatTimeToDeadlineString = ({ days, hours, minutes }: {
   days: number;
   hours: number;
   minutes: number;
+ }, options?: {
+   forceHours?: boolean;
+   forceMinutes?: boolean;
 }): string => {
-  if (days !== 0) return `${days}일`;
-  if (hours !== 0) return `${hours}시간`;
-  return `${minutes}분`;
+  if (days !== 0 && !options?.forceHours) return `${days}일`;
+  if ((hours !== 0 || options?.forceHours) && !options?.forceMinutes) return `${hours}시간`;
+  return `${minutes}분`;
 };
📝 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.

Suggested change
export const formatTimeToDeadlineString = ({ days, hours, minutes }: {
days: number;
hours: number;
minutes: number;
}): string => {
if (days !== 0) return `${days}일`;
if (hours !== 0) return `${hours}시간`;
return `${minutes}분`;
};
export const formatTimeToDeadlineString = ({ days, hours, minutes }: {
days: number;
hours: number;
minutes: number;
}, options?: {
forceHours?: boolean;
forceMinutes?: boolean;
}): string => {
if (days !== 0 && !options?.forceHours) return `${days}일`;
if ((hours !== 0 || options?.forceHours) && !options?.forceMinutes) return `${hours}시간`;
return `${minutes}분`;
};

Comment on lines +27 to +28
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Address the TODO comment about date management.

The comment indicates inconsistent date handling across the application. Consider implementing a consistent date management strategy.

Let's search for different date handling patterns in the codebase:


🏁 Script executed:

#!/bin/bash
# Search for date string to Date object conversions
rg "new Date\(" --type ts --type tsx

# Search for date formatting utilities
rg "format.*Time|Time.*format" --type ts --type tsx

Length of output: 148


Could you please run the above script to help us verify the date handling patterns across the codebase? This will allow us to determine if the inconsistency noted in the TODO comment still persists. Once you have the output, we can finalize the review comment accordingly.


🏁 Script executed:

[run_scripts]
#!/bin/bash
echo "Searching for date string to Date object conversions in TS and TSX files:"
rg "new Date\(" -g "*.ts" -g "*.tsx"

echo "Searching for date formatting utilities (e.g., formatTime) in TS and TSX files:"
rg "formatTime" -g "*.ts" -g "*.tsx"

Length of output: 16319


Ensure Consistent Date Management

In frontend/src/pages/CandidateSchedulePage/Header.tsx, the TODO comment highlights an ongoing inconsistency regarding date handling (i.e., using strings versus Date objects). Our codebase shows widespread use of date conversions via new Date() and utilities like formatTimeToColonString. To improve type safety and avoid potential runtime issues, please:

  • Unify Date Formats: Decide on a single standard (preferably working with Date objects) for date management across components.
  • Update Components & Types: Revise components such as DiscussionConfirmButton (and any others like HeaderTextInfo if applicable) to consistently expect and handle the chosen date type.
  • Document the Change: Once a unified strategy is adopted, remove or update the TODO comment accordingly.

@dioo1461 dioo1461 force-pushed the bugfix/fe/style-edit branch from 7b28c57 to 2cfa76a Compare February 21, 2025 05:01
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
frontend/src/utils/date/date.ts (1)

251-260: Add input validation and documentation to the new time calculation function.

The implementation correctly breaks down milliseconds into time units. However, consider these improvements:

  1. Add input validation for negative milliseconds
  2. Add JSDoc documentation
  3. Consider a more specific name like getDeadlineTimeInfo or getTimeRemainingInfo

Here's a suggested implementation:

-export const getTimeLeftInfoFromMilliseconds = (milliseconds: number) => {
+/**
+ * Breaks down a duration in milliseconds into days, hours, and minutes.
+ * @param milliseconds - The duration in milliseconds (must be non-negative)
+ * @returns An object containing the breakdown of time units
+ * @throws {Error} If milliseconds is negative
+ */
+export const getTimeLeftInfoFromMilliseconds = (milliseconds: number) => {
+  if (milliseconds < 0) {
+    throw new Error('milliseconds must be non-negative');
+  }
+
   const days = Math.floor(milliseconds / DAY_IN_MILLISECONDS);
   const remainingAfterDays = milliseconds % DAY_IN_MILLISECONDS;

   const hours = Math.floor(remainingAfterDays / HOUR_IN_MILLISECONDS);
   const remainingAfterHours = remainingAfterDays % HOUR_IN_MILLISECONDS;
   const minutes = Math.floor(remainingAfterHours / MINUTE_IN_MILLISECONDS);

   return { days, hours, minutes };
 };
frontend/src/features/discussion/ui/DiscussionConfirmButton/index.tsx (1)

23-24: Consider adding error handling for query invalidation.

The query invalidation is correctly implemented, but there's no error handling if the invalidation fails. Consider wrapping the invalidation calls in a try-catch block.

   const handleClickConfirm = () => {
     if (!param.id) return;
-    queryClient.invalidateQueries({ queryKey: upcomingQueryKey });
-    queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all });
+    try {
+      queryClient.invalidateQueries({ queryKey: upcomingQueryKey });
+      queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all });
+    } catch (error) {
+      console.error('Failed to invalidate queries:', error);
+      // Consider showing a user-friendly error message
+    }
     mutate({
       id: param.id, 
       body: { startDateTime, endDateTime }, 
     });
   };
frontend/src/pages/CandidateSchedulePage/Header.tsx (1)

21-25: Consider moving date conversion to HeaderTextInfo.

The date conversion logic should be encapsulated within the component that needs the Date objects. This would make the Header component more focused and improve separation of concerns.

-    <HeaderTextInfo
-      adjustCount={adjustCount}
-      endTime={new Date(endDateTime)}
-      startTime={new Date(startDateTime)}
-    />
+    <HeaderTextInfo
+      adjustCount={adjustCount}
+      endTime={endDateTime}
+      startTime={startDateTime}
+    />

// In HeaderTextInfo:
const HeaderTextInfo = ({ adjustCount, startTime, endTime }: {
  adjustCount: number;
-  startTime: Date;
-  endTime: Date;
+  startTime: string;
+  endTime: string;
}) => {
+  const startDate = new Date(startTime);
+  const endDate = new Date(endTime);
  // ...
  <Text typo='h2'>
-    {`${formatTimeToColonString(startTime)} ~ ${formatTimeToColonString(endTime)}`}
+    {`${formatTimeToColonString(startDate)} ~ ${formatTimeToColonString(endDate)}`}
  </Text>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b28c575eaace6d60d8b51c3f6c1d536f371a945 and 2cfa76a.

📒 Files selected for processing (8)
  • frontend/src/features/discussion/ui/DiscussionConfirmButton/index.tsx (2 hunks)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (3 hunks)
  • frontend/src/pages/CandidateSchedulePage/Header.tsx (1 hunks)
  • frontend/src/pages/CandidateSchedulePage/index.tsx (1 hunks)
  • frontend/src/pages/UpcomingScheduleDetailPage/Header.tsx (2 hunks)
  • frontend/src/utils/date/date.ts (2 hunks)
  • frontend/src/utils/date/format.ts (1 hunks)
  • frontend/src/utils/date/time.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • frontend/src/utils/date/time.ts
  • frontend/src/pages/UpcomingScheduleDetailPage/Header.tsx
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx
  • frontend/src/utils/date/format.ts
  • frontend/src/pages/CandidateSchedulePage/index.tsx
🔇 Additional comments (3)
frontend/src/utils/date/date.ts (1)

3-3: LGTM!

The additional imports are necessary and properly utilized in both the new and existing functions.

frontend/src/features/discussion/ui/DiscussionConfirmButton/index.tsx (1)

14-14:

Details

✅ Verification successful

Verify the route parameter pattern.

The route parameter pattern has been updated from /_main/discussion/$id to /_main/discussion/candidate/$id. Let's verify this change is consistent with the router configuration.


🏁 Script executed:

#!/bin/bash
# Search for route definitions in the codebase
rg -A 5 "createRoute|createRootRoute" --type ts --type tsx

Length of output: 88


Let's re-run the verification without the file type filters which appeared to be causing issues. Try the following command to search for references to the new route pattern in the repository:

This should help confirm whether the router configuration is using the updated /candidate/ pattern. Once you verify that the occurrences of /candidate/ exist in the router configuration as expected, please update the review comment or let us know if further adjustments are needed.


🏁 Script executed:

#!/bin/bash
# Locate any references to the new route pattern in the repository
rg "/_main/discussion/candidate/"

Length of output: 453


Route parameter pattern verified and consistent across the codebase.

  • The usage of useParams({ from: '/_main/discussion/candidate/$id' }) in frontend/src/features/discussion/ui/DiscussionConfirmButton/index.tsx is in line with the updated router configuration.
  • Similar usage in frontend/src/pages/CandidateSchedulePage/index.tsx and the route declaration in frontend/src/routes/_main/discussion/candidate/$id.lazy.tsx confirms that the new /candidate/$id pattern is applied consistently.
frontend/src/pages/CandidateSchedulePage/Header.tsx (1)

27-28: Address the TODO comment about date management.

The codebase shows inconsistent date handling patterns:

  • HeaderProps accepts dates as strings
  • HeaderTextInfo expects Date objects
  • Conversion happens in the parent component

This matches the concerns raised in the previous review.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleListItem.tsx (1)

57-69: Consider these improvements to the Deadline component.

The component logic is well-implemented and correctly handles deadline calculations. However, consider the following improvements:

  1. Add TypeScript interface for the props
  2. Extract the magic number "3" as a named constant
  3. Consider i18n for the Korean text strings
  4. Consider moving the component to a separate file

Here's a suggested implementation:

+interface DeadlineProps {
+  timeLeft: number;
+}
+
+const DEADLINE_WARNING_THRESHOLD_DAYS = 3;
+
-const Deadline = ({ timeLeft }: { timeLeft: number }) => {
+const Deadline = ({ timeLeft }: DeadlineProps) => {
  const timeLeftInfo = getTimeLeftInfoFromMilliseconds(timeLeft);
-  if (timeLeftInfo.days > 3) return null;
+  if (timeLeftInfo.days > DEADLINE_WARNING_THRESHOLD_DAYS) return null;
  const isExpired = timeLeft < 0;
  return (
    <Text color={vars.color.Ref.Red[500]} typo='b3M'>
      {isExpired ? 
-        `마감 기한이 ${formatTimeToDeadlineString(timeLeftInfo)} 지났어요`
+        t('deadline.expired', { time: formatTimeToDeadlineString(timeLeftInfo) })
        : 
-        `마감 기한까지 ${formatTimeToDeadlineString(timeLeftInfo)} 남았어요!`}
+        t('deadline.remaining', { time: formatTimeToDeadlineString(timeLeftInfo) })}
    </Text>
  );
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2cfa76a and a7f0220.

📒 Files selected for processing (3)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleListItem.tsx (3 hunks)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (3 hunks)
  • frontend/src/routes/_main/home/index.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • frontend/src/routes/_main/home/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx
🔇 Additional comments (2)
frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleListItem.tsx (2)

6-7: LGTM! New date utility imports.

The added imports provide the necessary functions for accurate deadline calculations, aligning with the PR's objective to fix deadline-related issues.


39-39: LGTM! Clean integration of Deadline component.

The Deadline component is well-integrated within the existing layout structure, receiving the necessary timeLeft prop from the schedule object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants