Skip to content

Conversation

@dioo1461
Copy link
Contributor

@dioo1461 dioo1461 commented Feb 25, 2025

#️⃣ 연관된 이슈>

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

  • UpcomingSchedule과 FinishedSchedule에서 공통으로 사용해야 할 SharedEventDto가 개별적으로 구현되어 있는 문제가 있었습니다.
  • 이를 하나의 공통된 인터페이스로 통합하여 중복을 제거하였습니다.

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

Summary by CodeRabbit

  • New Features

    • Enhanced Pagination: Pagination controls are now displayed based on the number of pages, improving user navigation.
    • Discussion Invitation Handling: A new component for managing discussion invitations has been introduced, allowing for password protection and user feedback.
    • Timer Button: A new timer button component has been added to track time until an event, enhancing user interaction.
    • Countdown Hook: A new custom hook for managing countdowns has been introduced, improving time management features.
    • Time Unlock Feature: A new field for optional time unlocking has been added to discussion invitations.
  • Bug Fixes

    • Null Handling in Schedules: Improved handling of null values in shared event data, ensuring robust validation and display.
  • Style

    • New Button Style: Introduced a new style for the timer button, enhancing visual consistency.

@dioo1461 dioo1461 requested a review from hamo-o as a code owner February 25, 2025 02:17
@dioo1461 dioo1461 added the 🖥️ FE Frontend label Feb 25, 2025
@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2025

Walkthrough

This pull request introduces adjustments to pagination rendering and shared event type validation. The Pagination component now renders its controls only when more than one page is available, while the OngoingScheduleList always displays the pagination component. Additionally, a new file defines a validated shared event schema with Zod, replacing inline schema definitions in related files. UI components and models have been updated to handle nullable shared events and to correctly import and utilize the centralized schema.

Changes

File(s) Change Summary
frontend/src/components/Pagination/index.tsx
frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx
Updated pagination logic: activated conditional rendering in Pagination and removed redundant condition in OngoingScheduleList.
frontend/src/features/shared-schedule/model/SharedEventDto.ts
frontend/src/features/shared-schedule/model/finishedSchedules.ts
frontend/src/features/shared-schedule/model/upcomingSchedules.ts
Introduced new SharedEventDto.ts to define a Zod schema for shared events; updated finished and upcoming schedules to import the centralized schema and, in finished schedules, allow null values for the shared event field.
frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx Modified type definitions in the FinishedScheduleListItem and MeetDate to allow a nullable shared event; updated date formatting and corrected the import path for the shared event type.
frontend/src/features/discussion/model/invitation.ts Added timeUnlocked field to InvitationResponseSchema, allowing it to be a date or null.
frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx
frontend/src/features/discussion/ui/DiscussionInviteCard/TimerButton.tsx
frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts
frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx
frontend/src/pages/DiscussionPage/DiscussionInvitePage/index.tsx
Introduced new components and styles for handling discussion invitations, including SubmitForm and TimerButton, and updated DiscussionInviteCard to delegate invitation handling to SubmitForm.
frontend/src/utils/date/time.ts Added constant SECOND_IN_MILLISECONDS to represent milliseconds in one second.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant OngoingScheduleList
    participant Pagination
    User->>OngoingScheduleList: Request schedule list
    OngoingScheduleList->>Pagination: Render Pagination component
    Pagination->>Pagination: Check if totalPages > 1
    alt totalPages > 1
        Pagination->>User: Display pagination controls
    else
        Pagination->>User: Skip rendering controls
    end
Loading

Possibly related PRs

  • [FE-Fix] 홈 화면 관련 버그 수정 및 UX 개선 반영 #279: The changes in the main PR, which modify the conditional rendering logic in the Pagination component, are related to the retrieved PR that introduces a commented-out conditional rendering line for the same component, indicating a direct connection in their modifications.

Suggested labels

🔮 ALL

Suggested reviewers

  • kwon204
  • efdao

Poem

I'm a bunny dancing through the lines of code,
Hopping over logic where pagination once strode.
Schemas now bloom with a validated light,
Shared events prance, nullable in flight.
With every change, my whiskers twitch with delight!
CodeRabbit Inc. cheers on every byte!


📜 Recent review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 57f9330 and bc070a5.

📒 Files selected for processing (1)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx

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.
  • @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.

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/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (1)

37-37: Address the TODO comment regarding useEffect.

The comment suggests that useEffect could be replaced. Consider using React Query's built-in state management or React's useState if the effect is for simple state management.

Would you like me to suggest alternative approaches to replace the useEffect?

frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx (2)

18-19: Remove commented-out code.

The startDate and endDate props are commented out and no longer used. Remove them to maintain clean code.

-  // startDate: Date;
-  // endDate: Date;

58-61: Add error handling for invalid date strings.

The current implementation assumes valid date strings. Consider adding error handling:

-    {sharedEventDto ? getDateTimeRangeString(
-      new Date(sharedEventDto.startDateTime), 
-      new Date(sharedEventDto.endDateTime))
+    {sharedEventDto ? (() => {
+      try {
+        return getDateTimeRangeString(
+          new Date(sharedEventDto.startDateTime),
+          new Date(sharedEventDto.endDateTime)
+        );
+      } catch (error) {
+        console.error('Invalid date format:', error);
+        return '날짜 형식이 잘못되었습니다';
+      }
+    })()
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d88c128 and a8600f2ad6f6ae97cdf9f7132708fc710a911d02.

📒 Files selected for processing (6)
  • frontend/src/components/Pagination/index.tsx (1 hunks)
  • frontend/src/features/shared-schedule/model/SharedEventDto.ts (1 hunks)
  • frontend/src/features/shared-schedule/model/finishedSchedules.ts (1 hunks)
  • frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1 hunks)
  • frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx (3 hunks)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • frontend/src/features/shared-schedule/model/upcomingSchedules.ts
  • frontend/src/features/shared-schedule/model/SharedEventDto.ts
🔇 Additional comments (3)
frontend/src/components/Pagination/index.tsx (1)

29-29: LGTM! Improved UX by conditionally rendering pagination.

The addition of the totalPages > 1 condition ensures that pagination controls are only shown when there are multiple pages, providing a cleaner user interface.

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

60-70: LGTM! Enhanced pagination with prefetching and state management.

The implementation includes performance optimization through prefetching and proper state management by resetting selectedIndex on page change.

frontend/src/features/shared-schedule/model/finishedSchedules.ts (1)

3-3: LGTM! Improved schema organization and type safety.

Good improvements:

  • Centralized SharedEventDtoSchema reduces duplication
  • Explicit null handling in the schema improves type safety

Also applies to: 9-9

@dioo1461 dioo1461 force-pushed the bugfix/fe/finished-schedule-zod-error branch from a8600f2 to 82c8ec9 Compare February 25, 2025 07:56
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 (8)
frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts (1)

61-64: Add unit to width property

The width property is specified as a numeric value without units. While some CSS-in-JS libraries may default to pixels, it's best practice to explicitly include the unit for clarity.

export const timerButtonStyle = style({
  justifyContent: 'center',
-  width: 126.77,
+  width: '126.77px',
});
frontend/src/features/discussion/ui/DiscussionInviteCard/TimerButton.tsx (2)

37-37: Enhance time display format for better readability

Currently, the timer displays time only in seconds, which may not be intuitive for users when longer times are displayed. Consider formatting the time in a more readable format like minutes:seconds.

- {`${Math.floor(remainingTime / 1000)}초`}
+ {`${Math.floor((remainingTime / 1000) / 60)}분 ${Math.floor((remainingTime / 1000) % 60)}초`}

Or alternatively, use a utility function for a cleaner implementation:

const formatTime = (ms: number): string => {
  const seconds = Math.floor(ms / 1000);
  const minutes = Math.floor(seconds / 60);
  const remainingSeconds = seconds % 60;
  return `${minutes}${remainingSeconds}초`;
};

// Then in the render:
{formatTime(remainingTime)}

19-29: Consider potential performance optimization

The interval is set to update every second, which is fine for most cases. However, for performance optimization, you could consider reducing the update frequency when the remaining time is large (e.g., update every minute when more than an hour remains).

frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (4)

12-12: Remove unnecessary semicolon.

There is a stray semicolon on line 12 that should be removed.

-import TimerButton from './TimerButton';
-;
+import TimerButton from './TimerButton';

46-50: Add validation for password format.

The placeholder suggests a numeric password of 4-6 digits, but there's no validation to enforce this requirement. Consider adding proper input type and pattern validation.

<input
  className={inputStyle}
  onChange={(e) => setPassword(e.target.value)}
  placeholder='숫자 4~6자리 비밀번호'
+  type="password"
+  pattern="[0-9]{4,6}"
+  maxLength={6}
+  minLength={4}
+  inputMode="numeric"
/>

38-38: Add user notification for timeout.

When an error occurs, you set a 5-minute timeout but don't inform the user. Consider adding a notification to explain this timeout to the user.

onError: () => {
  setUnlockDT(new Date(Date.now() + 5 * MINUTE_IN_MILLISECONDS)); 
+  addNoti({ 
+    type: 'error', 
+    title: '오류가 발생했습니다',
+    description: '5분 후에 다시 시도해주세요.'
+  });
},

28-28: Consider adding password validation when submitting.

You're submitting an empty password as undefined, but it might be better to validate the password format before submission, especially since the placeholder indicates a specific format requirement.

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

28-28: Remove or relocate TODO comment.

This TODO comment about input validation is now misplaced since the input handling has been moved to the SubmitForm component. Consider removing it or moving it to the appropriate location.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between a8600f2ad6f6ae97cdf9f7132708fc710a911d02 and 82c8ec9f0119bf1c923a5da6c7398fe7cc582692.

📒 Files selected for processing (13)
  • frontend/src/components/Pagination/index.tsx (1 hunks)
  • frontend/src/features/discussion/model/invitation.ts (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/TimerButton.tsx (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (3 hunks)
  • frontend/src/features/shared-schedule/model/SharedEventDto.ts (1 hunks)
  • frontend/src/features/shared-schedule/model/finishedSchedules.ts (1 hunks)
  • frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1 hunks)
  • frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx (3 hunks)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (1 hunks)
  • frontend/src/pages/DiscussionPage/DiscussionInvitePage/index.tsx (2 hunks)
  • frontend/src/utils/date/time.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • frontend/src/utils/date/time.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • frontend/src/features/shared-schedule/model/SharedEventDto.ts
  • frontend/src/components/Pagination/index.tsx
  • frontend/src/features/shared-schedule/model/upcomingSchedules.ts
  • frontend/src/features/shared-schedule/model/finishedSchedules.ts
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx
  • frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx
🔇 Additional comments (7)
frontend/src/features/discussion/model/invitation.ts (1)

15-15: LGTM: Well-structured schema addition

The new timeUnlocked field with the union type properly handles both date values and null cases.

frontend/src/pages/DiscussionPage/DiscussionInvitePage/index.tsx (1)

18-18: LGTM: Proper prop passing implementation

The timeUnlocked property is correctly destructured from the invitation object and properly passed down to the DiscussionInviteCard component.

Also applies to: 30-30

frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (2)

21-60: Good component separation.

The separation of the form logic into its own component improves modularity and reusability. The component handles state management and API interactions cleanly.


62-78: Well-designed conditional rendering.

The JoinButton component elegantly handles the conditional rendering between a regular button and a TimerButton based on the unlock date/time.

frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (3)

77-82: Good refactoring using the new SubmitForm component.

The integration of the SubmitForm component simplifies this component and improves modularity by properly passing all required props.


19-19: Consistent use of the timeUnlocked prop.

Good job adding the timeUnlocked prop to both interfaces and passing it through the component hierarchy to the SubmitForm component where it's needed.

Also applies to: 66-66


30-46: Cleaner component implementation.

The refactoring of DiscussionInviteCard to use the new SubmitForm component has resulted in a cleaner, more focused component that handles only the presentation aspects while delegating form functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

p2; 아무것도 입력하지 않으면 바로 400이 와서 락 하지 않고 따로 처리해줘야 할 것 같습니다.

@dioo1461 dioo1461 force-pushed the bugfix/fe/finished-schedule-zod-error branch from 82c8ec9 to d875a58 Compare February 27, 2025 15:31
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 (4)
frontend/src/utils/error/HTTPError.ts (1)

21-22: Good addition for rate limiting detection!

The implementation of isTooManyRequestsError() follows the same pattern as existing methods and correctly identifies HTTP 429 errors (Too Many Requests). This is useful for handling rate limiting scenarios in API calls.

Consider adding a descriptive comment to clarify that 429 represents "Too Many Requests" for better code documentation:

+  // Checks if the error is a 429 Too Many Requests error (rate limiting)
  isTooManyRequestsError = () => this.#status === 429;
frontend/src/hooks/useCountdown.ts (2)

11-21: Consider adding debounce mechanism for performance optimization

For performance optimization, especially in components that might re-render frequently, consider adding a debounce mechanism to prevent excessive updates when the countdown is far from zero.

useEffect(() => {
+   // Only update frequently when close to deadline
+   const updateInterval = remaining < 60000 ? SECOND_IN_MILLISECONDS : 10 * SECOND_IN_MILLISECONDS;
    const interval = setInterval(() => {
      const remaining = Math.max(targetDateTime.getTime() - Date.now(), 0);
      setRemainingTime(remaining);
      if (remaining === 0) {
        clearInterval(interval);
        onTimeEnd?.();
      }
-   }, SECOND_IN_MILLISECONDS);
+   }, updateInterval);

    return () => clearInterval(interval);
  }, [targetDateTime, onTimeEnd]);

5-24: Consider returning a structured time object instead of raw milliseconds

Currently, the hook returns raw milliseconds which requires consumers to perform their own conversions. Consider enhancing the return value to include a structured format with hours, minutes, and seconds for easier consumption.

const useCountdown = (targetDateTime: Date, onTimeEnd?: () => void): number => {
-  const [remainingTime, setRemainingTime] = useState<number>(
+  const [remainingTime, setRemainingTime] = useState<number>(
    Math.max(targetDateTime.getTime() - Date.now(), 0),
  );

  useEffect(() => {
    const interval = setInterval(() => {
      const remaining = Math.max(targetDateTime.getTime() - Date.now(), 0);
      setRemainingTime(remaining);
      if (remaining === 0) {
        clearInterval(interval);
        onTimeEnd?.();
      }
    }, SECOND_IN_MILLISECONDS);

    return () => clearInterval(interval);
  }, [targetDateTime, onTimeEnd]);

  return remainingTime;
+
+  // Alternative enhanced return type
+  // const seconds = Math.floor((remainingTime / 1000) % 60);
+  // const minutes = Math.floor((remainingTime / (1000 * 60)) % 60);
+  // const hours = Math.floor(remainingTime / (1000 * 60 * 60));
+  // return { 
+  //   total: remainingTime,
+  //   hours,
+  //   minutes,
+  //   seconds
+  // };
};
frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (1)

69-79: Add accessibility attributes to the button

To improve accessibility, consider adding ARIA attributes and proper labels to the button component.

const JoinButton = ({ canJoin, onClick, initialUnlockDateTime, onTimeEnd }: { 
  canJoin: boolean;
  onClick: () => void;
  onTimeEnd: () => void;
  initialUnlockDateTime: Date | null;
}) => (
  initialUnlockDateTime === null ?
    <Button
      disabled={!canJoin}
      onClick={onClick}
      size='xl'
+     aria-label="초대 수락하기"
+     role="button"
    >
      초대 수락하기
    </Button>
    :
    <TimerButton onTimeEnd={onTimeEnd} targetDateTime={initialUnlockDateTime} />
);
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 82c8ec9f0119bf1c923a5da6c7398fe7cc582692 and d875a58.

📒 Files selected for processing (15)
  • frontend/src/components/Pagination/index.tsx (1 hunks)
  • frontend/src/features/discussion/model/invitation.ts (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/TimerButton.tsx (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (3 hunks)
  • frontend/src/features/shared-schedule/model/SharedEventDto.ts (1 hunks)
  • frontend/src/features/shared-schedule/model/finishedSchedules.ts (1 hunks)
  • frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1 hunks)
  • frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx (3 hunks)
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx (1 hunks)
  • frontend/src/hooks/useCountdown.ts (1 hunks)
  • frontend/src/pages/DiscussionPage/DiscussionInvitePage/index.tsx (2 hunks)
  • frontend/src/utils/date/time.ts (1 hunks)
  • frontend/src/utils/error/HTTPError.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts
  • frontend/src/utils/date/time.ts
  • frontend/src/features/shared-schedule/model/SharedEventDto.ts
  • frontend/src/features/discussion/model/invitation.ts
  • frontend/src/features/shared-schedule/model/finishedSchedules.ts
  • frontend/src/components/Pagination/index.tsx
  • frontend/src/features/shared-schedule/ui/OngoingSchedules/OngoingScheduleList.tsx
  • frontend/src/pages/DiscussionPage/DiscussionInvitePage/index.tsx
  • frontend/src/features/discussion/ui/DiscussionInviteCard/TimerButton.tsx
  • frontend/src/features/shared-schedule/model/upcomingSchedules.ts
  • frontend/src/features/shared-schedule/ui/FinishedSchedules/FinishedScheduleListItem.tsx
🔇 Additional comments (7)
frontend/src/hooks/useCountdown.ts (1)

5-24: Well-implemented countdown hook with proper cleanup

The hook is well-designed, with appropriate state management and cleanup handling. The use of useEffect with the correct dependencies ensures the timer updates properly and is cleaned up when needed.

frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (4)

12-12: Good refactoring to extract form logic

The import of the new SubmitForm component aligns well with the PR objective to reduce duplication and improve modularity.


19-19: Appropriately added timeUnlocked prop

Adding the timeUnlocked property of type Date | null properly supports the new functionality of timed access control.


31-46: Clean component simplification with proper prop passing

The refactor has simplified the DiscussionInviteCard component by delegating form handling to the SubmitForm component. The props are destructured and passed appropriately.


77-82: Good implementation of the extracted SubmitForm component

The integration of the SubmitForm component with proper prop passing improves the component architecture and follows the single responsibility principle.

frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (2)

28-29: Handle empty password input correctly

The code converts empty password input to undefined, which is good practice, but you should validate the password format before submission as noted in a previous comment.

According to the previous review, there's an issue where empty password input causes a 400 error. Consider adding validation before submission to prevent this error.


36-41: Good error handling with retry lockout

The error handling correctly identifies too many request errors and implements a 5-minute lockout period, improving security against brute force attacks.

Comment on lines +55 to +61
const PasswordInput = ({ onChange }: { onChange: (password: string) => void }) => (
<input
className={inputStyle}
onChange={(e) => onChange(e.target.value)}
placeholder='숫자 4~6자리 비밀번호'
/>
);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation for password format

The input placeholder indicates "숫자 4~6자리 비밀번호" (4-6 digit password), but there's no validation to enforce this constraint. Consider adding validation to match the expected format.

const PasswordInput = ({ onChange }: { onChange: (password: string) => void }) => (
  <input
    className={inputStyle}
+   type="password"
+   pattern="[0-9]{4,6}"
+   maxLength={6}
    onChange={(e) => onChange(e.target.value)}
    placeholder='숫자 4~6자리 비밀번호'
  />
);
📝 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
const PasswordInput = ({ onChange }: { onChange: (password: string) => void }) => (
<input
className={inputStyle}
onChange={(e) => onChange(e.target.value)}
placeholder='숫자 4~6자리 비밀번호'
/>
);
const PasswordInput = ({ onChange }: { onChange: (password: string) => void }) => (
<input
className={inputStyle}
type="password"
pattern="[0-9]{4,6}"
maxLength={6}
onChange={(e) => onChange(e.target.value)}
placeholder='숫자 4~6자리 비밀번호'
/>
);

Comment on lines 21 to 53
const SubmitForm = ({ discussionId, requirePW, canJoin, unlockDateTime }: SubmitFormProps) => {
const navigate = useNavigate();
const { mutate } = useInvitationJoinMutation();
const [password, setPassword] = useState('');
const [unlockDT, setUnlockDT] = useState<Date | null>(unlockDateTime);
const handleJoinClick = () => {
mutate(
{ body: { discussionId, password: password === '' ? undefined : password } },
{ onSuccess: (data) => {
if (data.isSuccess) {
navigate({ to: '/discussion/$id', params: { id: discussionId.toString() } });
} else {
addNoti({ type: 'error', title: `비밀번호가 일치하지 않습니다 - ${data.failedCount}회 시도` });
}
},
onError: (error: Error) => {
if (error instanceof HTTPError && error.isTooManyRequestsError()) {
setUnlockDT(new Date(Date.now() + 5 * MINUTE_IN_MILLISECONDS));
}
} });
};
return (
<Flex align='flex-end' gap={500}>
{requirePW && canJoin && <PasswordInput onChange={setPassword} />}
<JoinButton
canJoin={canJoin}
initialUnlockDateTime={unlockDT}
onClick={handleJoinClick}
onTimeEnd={() => setUnlockDT(null)}
/>
</Flex>
);
};
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation before submission

The current implementation doesn't validate the password format before submission. Consider adding validation to ensure the password meets the required format (4-6 digits) before submitting the form.

const SubmitForm = ({ discussionId, requirePW, canJoin, unlockDateTime }: SubmitFormProps) => {
  const navigate = useNavigate();
  const { mutate } = useInvitationJoinMutation();
  const [password, setPassword] = useState('');
+ const [passwordError, setPasswordError] = useState<string | null>(null);
  const [unlockDT, setUnlockDT] = useState<Date | null>(unlockDateTime);
+ 
+ const validatePassword = () => {
+   if (!requirePW || password === '') return true;
+   const isValid = /^\d{4,6}$/.test(password);
+   if (!isValid) {
+     setPasswordError('비밀번호는 4~6자리 숫자여야 합니다');
+   } else {
+     setPasswordError(null);
+   }
+   return isValid;
+ };
+
  const handleJoinClick = () => {
+   if (!validatePassword()) return;
+
    mutate(
      { body: { discussionId, password: password === '' ? undefined : password } },
      { onSuccess: (data) => {
        if (data.isSuccess) {
          navigate({ to: '/discussion/$id', params: { id: discussionId.toString() } });
        } else {
          addNoti({ type: 'error', title: `비밀번호가 일치하지 않습니다 - ${data.failedCount}회 시도` });
        }
      },
      onError: (error: Error) => {
        if (error instanceof HTTPError && error.isTooManyRequestsError()) {
          setUnlockDT(new Date(Date.now() + 5 * MINUTE_IN_MILLISECONDS)); 
        }
      } });
  };
  return (
    <Flex align='flex-end' gap={500}>
-     {requirePW && canJoin && <PasswordInput onChange={setPassword} />}
+     {requirePW && canJoin && (
+       <Flex direction="column" gap={100}>
+         <PasswordInput onChange={setPassword} />
+         {passwordError && <Text color={vars.color.Ref.Red[500]} typo="b3M">{passwordError}</Text>}
+       </Flex>
+     )}
      <JoinButton
        canJoin={canJoin}
        initialUnlockDateTime={unlockDT}
        onClick={handleJoinClick}
        onTimeEnd={() => setUnlockDT(null)}
      />
    </Flex>
  ); 
};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const SubmitForm = ({ discussionId, requirePW, canJoin, unlockDateTime }: SubmitFormProps) => {
const navigate = useNavigate();
const { mutate } = useInvitationJoinMutation();
const [password, setPassword] = useState('');
const [unlockDT, setUnlockDT] = useState<Date | null>(unlockDateTime);
const handleJoinClick = () => {
mutate(
{ body: { discussionId, password: password === '' ? undefined : password } },
{ onSuccess: (data) => {
if (data.isSuccess) {
navigate({ to: '/discussion/$id', params: { id: discussionId.toString() } });
} else {
addNoti({ type: 'error', title: `비밀번호가 일치하지 않습니다 - ${data.failedCount}회 시도` });
}
},
onError: (error: Error) => {
if (error instanceof HTTPError && error.isTooManyRequestsError()) {
setUnlockDT(new Date(Date.now() + 5 * MINUTE_IN_MILLISECONDS));
}
} });
};
return (
<Flex align='flex-end' gap={500}>
{requirePW && canJoin && <PasswordInput onChange={setPassword} />}
<JoinButton
canJoin={canJoin}
initialUnlockDateTime={unlockDT}
onClick={handleJoinClick}
onTimeEnd={() => setUnlockDT(null)}
/>
</Flex>
);
};
const SubmitForm = ({ discussionId, requirePW, canJoin, unlockDateTime }: SubmitFormProps) => {
const navigate = useNavigate();
const { mutate } = useInvitationJoinMutation();
const [password, setPassword] = useState('');
const [passwordError, setPasswordError] = useState<string | null>(null);
const [unlockDT, setUnlockDT] = useState<Date | null>(unlockDateTime);
const validatePassword = () => {
if (!requirePW || password === '') return true;
const isValid = /^\d{4,6}$/.test(password);
if (!isValid) {
setPasswordError('비밀번호는 4~6자리 숫자여야 합니다');
} else {
setPasswordError(null);
}
return isValid;
};
const handleJoinClick = () => {
if (!validatePassword()) return;
mutate(
{ body: { discussionId, password: password === '' ? undefined : password } },
{ onSuccess: (data) => {
if (data.isSuccess) {
navigate({ to: '/discussion/$id', params: { id: discussionId.toString() } });
} else {
addNoti({ type: 'error', title: `비밀번호가 일치하지 않습니다 - ${data.failedCount}회 시도` });
}
},
onError: (error: Error) => {
if (error instanceof HTTPError && error.isTooManyRequestsError()) {
setUnlockDT(new Date(Date.now() + 5 * MINUTE_IN_MILLISECONDS));
}
} });
};
return (
<Flex align='flex-end' gap={500}>
{requirePW && canJoin && (
<Flex direction="column" gap={100}>
<PasswordInput onChange={setPassword} />
{passwordError && <Text color={vars.color.Ref.Red[500]} typo="b3M">{passwordError}</Text>}
</Flex>
)}
<JoinButton
canJoin={canJoin}
initialUnlockDateTime={unlockDT}
onClick={handleJoinClick}
onTimeEnd={() => setUnlockDT(null)}
/>
</Flex>
);
};

Copy link
Contributor

@hamo-o hamo-o left a comment

Choose a reason for hiding this comment

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

5번째가 아니라 6번째에 타이머가 동작하네요..!

@dioo1461
Copy link
Contributor Author

dioo1461 commented Mar 3, 2025

5번째가 아니라 6번째에 타이머가 동작하네요..!

ea3660c
반영했습니다~

Comment on lines 36 to 38
} else {
setUnlockDT(new Date(Date.now() + LOCK_TIME_IN_MILLISECONDS));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

p2; 여기서도 노티는 띄워야 할 것 같습니다..! 사용자 입장에서 경고메시지 없이 잠기면 당황스러울 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

인정합니다 반영해 볼께요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

에러 메시지를 다음과 같이 노출하도록 해 보았는데 괜찮나요?

  • 5회 미만 실패 시
image
  • 5회 실패 시
image

Copy link
Contributor

Choose a reason for hiding this comment

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

오 좋네요!!

@hamo-o hamo-o merged commit 741618c into dev Mar 5, 2025
1 check passed
@hamo-o hamo-o deleted the bugfix/fe/finished-schedule-zod-error branch March 5, 2025 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🖥️ FE Frontend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants