[REFACTOR] 로그아웃 확인 모달 추가 #305
Conversation
young-jin-son
added
refactor
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LogoutButton
participant Drawer
participant HomeModal
participant LogoutMutation
User->>LogoutButton: Clicks Logout
LogoutButton->>Drawer: closeDrawer()
LogoutButton->>HomeModal: Open confirmation modal
User->>HomeModal: Confirm logout
HomeModal->>LogoutButton: onConfirm
LogoutButton->>LogoutMutation: Execute logout
LogoutButton->>HomeModal: Close modal
User->>HomeModal: Cancel
HomeModal->>LogoutButton: onClose
LogoutButton->>HomeModal: Close modal
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Button/LogoutButton/index.tsx(1 hunks)
🔇 Additional comments (5)
frontend/src/components/Button/LogoutButton/index.tsx (5)
10-12: LGTM! Proper hooks usage and state management.The state management and hooks usage follow React best practices with clear separation of concerns.
14-17: LGTM! Clear handler with proper separation of concerns.The logout click handler properly separates the drawer closing and modal opening actions.
24-26: LGTM! Clean modal close handler.Simple and effective modal close handler.
28-42: LGTM! Clean render logic with proper conditional rendering.The component render logic is well-structured with appropriate conditional rendering for the modal. The modal props are clearly defined and consistently used.
Note: The
contentprop uses an array format which appears to be specific to theHomeModalcomponent's expected interface.
1-42: HomeModal is already a generic, reusable componentThe
HomeModalimplementation lives underfrontend/src/pages/PlanPage/components/HomeModaland is used in multiple contexts (PlanPage, MainPage, DeleteAccountButton, LogoutButton) with only generic props (title,content,close,onConfirm, optionalconfirmText). No further decoupling or refactoring is required.
| const handleConfirmLogout = () => { | ||
| logoutMutation(); | ||
| setIsModalOpen(false); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and operation order.
The current implementation calls logoutMutation() before closing the modal, which could lead to UI inconsistencies if the logout fails. Consider handling the logout result or reordering operations.
const handleConfirmLogout = () => {
- logoutMutation();
- setIsModalOpen(false);
+ setIsModalOpen(false);
+ logoutMutation();
};Or better yet, handle the logout result:
const handleConfirmLogout = async () => {
+ try {
+ await logoutMutation();
setIsModalOpen(false);
+ } catch (error) {
+ // Handle logout error - maybe show error message
+ console.error('Logout failed:', error);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleConfirmLogout = () => { | |
| logoutMutation(); | |
| setIsModalOpen(false); | |
| }; | |
| // Before: | |
| // const handleConfirmLogout = () => { | |
| // logoutMutation(); | |
| // setIsModalOpen(false); | |
| // }; | |
| // Improved: handle the logout result and only close the modal on success | |
| const handleConfirmLogout = async () => { | |
| try { | |
| await logoutMutation(); | |
| setIsModalOpen(false); | |
| } catch (error) { | |
| // Handle logout error - maybe show an error message to the user | |
| console.error('Logout failed:', error); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/Button/LogoutButton/index.tsx around lines 19 to 22,
the logoutMutation is called before closing the modal, which may cause UI
inconsistencies if logout fails. Refactor handleConfirmLogout to await the
logoutMutation call and only close the modal if the logout succeeds.
Additionally, add error handling to manage any logout failures gracefully
without closing the modal prematurely.
| import * as S from './LogoutButton.styled'; | ||
|
|
||
| import { useDrawer } from '@/components/Drawer/hooks/useDrawer'; | ||
| import HomeModal from '@/pages/PlanPage/components/HomeModal'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider relocating the modal to a shared components directory.
Importing HomeModal from a page-specific directory (@/pages/PlanPage/components/HomeModal) into a generic button component creates tight coupling and violates component isolation principles. This makes the LogoutButton dependent on PlanPage components, which could lead to maintainability issues.
Consider one of these solutions:
- Move
HomeModalto a shared components directory (e.g.,@/components/Modal/HomeModal) - Create a generic confirmation modal component specifically for logout actions
- Use a global modal context or state management solution
-import HomeModal from '@/pages/PlanPage/components/HomeModal';
+import ConfirmationModal from '@/components/Modal/ConfirmationModal';📝 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.
| import HomeModal from '@/pages/PlanPage/components/HomeModal'; | |
| // frontend/src/components/Button/LogoutButton/index.tsx | |
| import React, { useState } from 'react'; | |
| import { useLogoutMutation } from '@/hooks/useLogoutMutation'; | |
| import { useDrawer } from '@/hooks/useDrawer'; | |
| -import HomeModal from '@/pages/PlanPage/components/HomeModal'; | |
| +import ConfirmationModal from '@/components/Modal/ConfirmationModal'; | |
| // …rest of the component… |
🤖 Prompt for AI Agents
In frontend/src/components/Button/LogoutButton/index.tsx at line 7, the import
of HomeModal from a page-specific directory creates tight coupling and breaks
component isolation. To fix this, move HomeModal to a shared components
directory such as @/components/Modal/HomeModal, or alternatively create a
generic confirmation modal for logout actions, or implement a global modal
context/state management to handle modals more cleanly and decouple LogoutButton
from PlanPage components.
Issue Number
close #304
As-Is
To-Be
Check List
Test Screenshot
(Optional) Additional Description
Summary by CodeRabbit
New Features
Enhancements