[FE-Feat] 논의 수정 & 삭제 구현 #394
Conversation
WalkthroughThis update introduces new API methods for editing and deleting discussions, adds corresponding React hooks for mutation operations, and refactors UI components to support discussion editing and deletion. The form logic is modularized, and the edit page is now data-driven, fetching discussion details dynamically. Several components are updated for state management and improved structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiscussionForm
participant API
participant Router
participant Modal
User->>DiscussionForm: Clicks Edit or Delete Button
alt Edit
DiscussionForm->>API: PUT /api/v1/discussion/{id}
API-->>DiscussionForm: Updated discussion data
DiscussionForm->>Router: Navigate to /discussion/{id}
else Delete
DiscussionForm->>Modal: Show confirmation modal
Modal->>User: User confirms
DiscussionForm->>API: DELETE /api/v1/discussion/{id}
API-->>DiscussionForm: Success
DiscussionForm->>Router: Navigate to home
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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 (
|
hamo-o
force-pushed
the
feature/fe/discussion-edit-delete
branch
from
f19acc5 to
6e2406d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/src/pages/DiscussionPage/DiscussionEditPage/index.tsx (1)
12-14: Consider adding error handlingWhile the loading state is handled appropriately, there's no specific handling for query errors. Consider adding error handling to provide feedback to users when the discussion data cannot be fetched.
- const { discussion, isPending } = useDiscussionQuery(id); + const { discussion, isPending, isError } = useDiscussionQuery(id); - if (isPending || !discussion) return null; + if (isPending) return <LoadingSpinner />; // Or your loading component + if (isError || !discussion) return <ErrorMessage message="토론을 불러오는 데 실패했습니다." />; // Or your error componentfrontend/src/features/discussion/ui/DiscussionForm/EnrollButton.tsx (2)
55-57: Consider adding loading state for deletionThe delete operation might take some time to complete. Consider adding a loading state to provide feedback to the user during the deletion process and prevent multiple clicks.
const { createModal, onModalClose } = useGlobalModal(); - const { mutate } = useDiscussionDeleteMutation(onModalClose); + const { mutate, isPending } = useDiscussionDeleteMutation(onModalClose); const handleClickDeleteDiscussion = () => { createModal({ // ... existing code ... children: ( <Flex align='flex-end' height='100%' justify='flex-end' > <Button - onClick={() => mutate()} + onClick={() => mutate()} + disabled={isPending} + isLoading={isPending} size='xl' variant='re' > 삭제하기 </Button> </Flex> ), }); };
6-10: Consider organizing imports alphabeticallyFor better readability and easier maintenance, consider organizing your imports alphabetically within each import section.
-import { - useDiscussionDeleteMutation, - useDiscussionEditMutation, - useDiscussionMutation, -} from '../../api/mutations'; +import { + useDiscussionDeleteMutation, + useDiscussionEditMutation, + useDiscussionMutation, +} from '../../api/mutations';frontend/src/features/discussion/api/mutations.ts (3)
32-51: Consider adding error handling to the edit mutation.The implementation of
useDiscussionEditMutationfollows good practices by extracting parameters from the route and invalidating relevant caches, including the specific discussion detail cache. However, there's no error handling in case the API call fails.export const useDiscussionEditMutation = () => { const { id } = useParams({ from: '/_main/discussion/edit/$id' }); const queryClient = useQueryClient(); const navigate = useNavigate(); - const { mutate } = useMutation({ + const { mutate, isError, error } = useMutation({ mutationFn: (body: DiscussionRequest) => discussionApi.putDiscussion(id, body), onSuccess: (_) => { queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); queryClient.invalidateQueries({ queryKey: discussionKeys.detail(id) }); navigate({ to: '/discussion/$id', params: { id }, }); }, + onError: (error) => { + // Handle error appropriately, maybe show a toast notification + console.error('Failed to update discussion:', error); + }, }); - return { mutate }; + return { mutate, isError, error }; };
53-68: Consider harmonizing the callback pattern across mutation hooks.The
useDiscussionDeleteMutationhook takes a callback as a parameter, while the other hooks don't use this pattern. This inconsistency might make the API harder to understand. Consider either:
- Using a similar callback pattern for all hooks, or
- Using the onSuccess pattern consistently and letting components handle additional logic
Additionally, consider adding error handling to this hook as well.
Two possible approaches:
- Make the callback optional with a default empty function:
-export const useDiscussionDeleteMutation = (callback: () => void) => { +export const useDiscussionDeleteMutation = (callback: () => void = () => {}) => {
- Alternatively, use options object for more flexibility:
-export const useDiscussionDeleteMutation = (callback: () => void) => { +export const useDiscussionDeleteMutation = (options?: { onSuccess?: () => void }) => { const { id } = useParams({ from: '/_main/discussion/edit/$id' }); const queryClient = useQueryClient(); const navigate = useNavigate(); const { mutate } = useMutation({ mutationFn: () => discussionApi.deleteDiscussion(id), onSuccess: (_) => { queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); - callback(); + options?.onSuccess?.(); navigate({ to: '/home' }); }, }); return { mutate }; };
12-68: Consider reducing code duplication in cache invalidation.There's a pattern of cache invalidation that's repeated across all three hooks. Consider extracting this into a utility function to reduce duplication and make it easier to maintain.
+// Add at the top of the file +const invalidateDiscussionCaches = (queryClient: any, discussionId?: string) => { + queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); + queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); + queryClient.invalidateQueries({ queryKey: discussionKeys.all }); + if (discussionId) { + queryClient.invalidateQueries({ queryKey: discussionKeys.detail(discussionId) }); + } +}; // Then in each hook, replace the multiple invalidateQueries calls with: // useDiscussionMutation - queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); - queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); - queryClient.invalidateQueries({ queryKey: discussionKeys.all }); + invalidateDiscussionCaches(queryClient); // useDiscussionEditMutation - queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); - queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); - queryClient.invalidateQueries({ queryKey: discussionKeys.detail(id) }); + invalidateDiscussionCaches(queryClient, id); // useDiscussionDeleteMutation - queryClient.invalidateQueries({ queryKey: upcomingQueryKey }); - queryClient.invalidateQueries({ queryKey: ongoingQueryKey.all }); + invalidateDiscussionCaches(queryClient);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between c391efc and 062962f8c5a38629dc08d1fd2fd789f43899658d.
📒 Files selected for processing (9)
frontend/src/features/discussion/api/index.ts(1 hunks)frontend/src/features/discussion/api/mutations.ts(2 hunks)frontend/src/features/discussion/api/queries.ts(1 hunks)frontend/src/features/discussion/ui/DiscussionEditButton/index.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionForm/EnrollButton.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionForm/FormButton.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionForm/index.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionTitle/index.tsx(1 hunks)frontend/src/pages/DiscussionPage/DiscussionEditPage/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
frontend/src/features/discussion/api/queries.ts (1)
frontend/src/features/discussion/model/index.ts (1)
DiscussionResponse(94-94)
frontend/src/features/discussion/ui/DiscussionForm/index.tsx (2)
frontend/src/features/discussion/ui/DiscussionForm/type.ts (1)
FormType(8-8)frontend/src/features/discussion/model/index.ts (1)
DiscussionRequest(92-92)
frontend/src/features/discussion/ui/DiscussionTitle/index.tsx (6)
frontend/src/features/discussion/api/queries.ts (1)
useDiscussionQuery(62-67)frontend/src/components/Flex/index.tsx (1)
Flex(20-59)frontend/src/features/discussion/ui/DiscussionTitle/index.css.ts (1)
titleStyle(5-7)frontend/src/components/Text/index.tsx (1)
Text(15-18)frontend/src/theme/index.css.ts (1)
vars(10-15)frontend/src/features/discussion/ui/DiscussionTitle/DiscussionBadges.tsx (1)
DiscussionBadges(8-38)
frontend/src/features/discussion/api/index.ts (2)
frontend/src/features/discussion/model/index.ts (2)
DiscussionRequest(92-92)DiscussionResponse(94-94)frontend/src/utils/fetch/index.ts (1)
request(19-30)
frontend/src/features/discussion/ui/DiscussionEditButton/index.tsx (2)
frontend/src/features/discussion/api/queries.ts (1)
useDiscussionHostQuery(105-110)frontend/src/components/Icon/component/Pencil.tsx (1)
Pencil(4-16)
frontend/src/features/discussion/ui/DiscussionForm/FormButton.tsx (3)
frontend/src/features/discussion/ui/DiscussionForm/type.ts (1)
FormType(8-8)frontend/src/features/discussion/ui/DiscussionForm/EnrollButton.tsx (3)
AddButton(14-32)DeleteButton(54-92)EditButton(34-52)frontend/src/components/Flex/index.tsx (1)
Flex(20-59)
🔇 Additional comments (16)
frontend/src/features/discussion/api/queries.ts (1)
63-66: Property naming updated for consistencyThe change from
isLoadingtoisPendingaligns with the React Query v4+ API, whereisPendingis the preferred flag for tracking loading state. This update ensures consistency across the codebase.frontend/src/features/discussion/ui/DiscussionForm/index.tsx (1)
18-19: LGTM! Improved formatting for readabilityThe type annotation for component props has been reformatted across multiple lines for better readability without changing functionality.
frontend/src/features/discussion/ui/DiscussionTitle/index.tsx (3)
8-8: LGTM! New import for edit button functionalityThe addition of the DiscussionEditButton import supports the discussion editing capability.
14-14: Updated loading state property nameProperty updated from
isLoadingtoisPendingto match the changes in the query hook implementation.
16-39: Improved layout to accommodate edit buttonThe layout has been restructured to:
- Use space-between justification for the main container
- Add a full-width setting to the container
- Nest the title and badges in their own Flex container
- Add the new DiscussionEditButton component
This creates a clean UI with the edit button properly aligned to the right of the discussion information.
frontend/src/features/discussion/api/index.ts (1)
27-37: Added API methods for discussion editing and deletionThe new
putDiscussionanddeleteDiscussionmethods correctly implement the API endpoints for updating and removing discussions. These additions complete the CRUD operations for the discussion feature.Both methods follow consistent patterns with the existing API implementation:
- Proper URL construction with the discussion ID
- Appropriate HTTP methods (PUT for update, DELETE for removal)
- Correct typing for request bodies and responses
frontend/src/features/discussion/ui/DiscussionEditButton/index.tsx (1)
1-28: Clean implementation of conditional edit buttonThis component is well-structured and follows good practices:
- Correctly uses
useParamsto extract the discussion ID from the URL- Properly checks if the current user is the host using
useDiscussionHostQuery- Handles loading state appropriately with
isPending- Only renders the button when the user is authorized (is the host)
- Uses appropriate styling and accessibility features
frontend/src/pages/DiscussionPage/DiscussionEditPage/index.tsx (2)
1-15: Good dynamic data fetching implementationThe page now properly fetches discussion data based on the URL parameter instead of using hardcoded values. The loading state handling is implemented correctly with the early return when data is still loading or unavailable.
16-40: Form initialization looks correctThe form is properly initialized with dynamic data from the fetched discussion object. All necessary fields are mapped correctly to the form's initialValues.
frontend/src/features/discussion/ui/DiscussionForm/FormButton.tsx (1)
3-19: Good separation of concernsThe refactoring of
FormButtonto a stateless component that delegates responsibilities to specialized button components is a good practice. This makes the component more focused and maintainable by following the single responsibility principle.frontend/src/features/discussion/ui/DiscussionForm/EnrollButton.tsx (3)
14-32: AddButton implementation looks goodThe
AddButtoncomponent correctly:
- Uses form context to access form state and validation
- Disables the button when the form is invalid
- Uses the appropriate mutation hook for creating discussions
34-52: EditButton implementation looks goodThe
EditButtoncomponent correctly:
- Uses form context to access form state and validation
- Disables the button when the form is invalid
- Uses the appropriate mutation hook for editing discussions
54-92: DeleteButton implementation is well doneThe
DeleteButtoncomponent properly:
- Integrates with the global modal system for confirmation
- Provides clear warning messages about the irreversible action
- Uses appropriate styling for a destructive action
- Closes the modal after the mutation is completed
frontend/src/features/discussion/api/mutations.ts (3)
2-2: Good use of tanstack router imports for consistency.Using
useNavigateanduseParamsfrom the same library (@tanstack/react-router) makes sense for consistency with other tanstack tools like React Query that you're already using in this file.
12-30: Well-refactored mutation hook with improved cache invalidation.The refactoring of
useDiscussionMutationremoves the callback parameter and instead handles navigation directly within the hook, which provides a more cohesive API and better encapsulation of related logic. The cache invalidation strategy for multiple queries is comprehensive.
32-51: Ensure type safety for the ID parameter.The ID extracted from route parameters is used directly in API calls. It's good practice to ensure this value is properly validated or converted to the expected type before using it in critical operations.
For example, if the API expects a numeric ID but the route parameter is a string, you might need to convert it:
// In useDiscussionEditMutation - mutationFn: (body: DiscussionRequest) => discussionApi.putDiscussion(id, body), + mutationFn: (body: DiscussionRequest) => { + // Ensure id is in the expected format + const numericId = parseInt(id, 10); + if (isNaN(numericId)) { + throw new Error('Invalid discussion ID'); + } + return discussionApi.putDiscussion(String(numericId), body); + }, // Similarly in useDiscussionDeleteMutationVerify how the discussionApi methods are defined to ensure you're passing the correct types.
Also applies to: 59-60
hamo-o
force-pushed
the
feature/fe/discussion-edit-delete
branch
from
062962f to
7ca4ea8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/apps/client/src/features/discussion/api/queries.ts (1)
82-86: Fix inconsistent loading state property usage.The
useDiscussionRankQueryhook still usesisLoadingwhile other hooks in this file useisPending. This creates inconsistency in the codebase.Apply this diff to maintain consistency:
- const { data: rank, isLoading } = useQuery<DiscussionRankResponse>( + const { data: rank, isPending } = useQuery<DiscussionRankResponse>( discussionRankQuery(discussionId, body), ); - return { rank, isLoading }; + return { rank, isPending };frontend/apps/client/src/features/discussion/ui/DiscussionForm/FormButton.tsx (1)
6-21: Well-structured presentational component!The refactoring to a purely presentational component improves separation of concerns. The conditional rendering based on form type is clear and maintainable.
Consider adding a comment or using a more descriptive prop name than
typeto clarify what FormType values are expected (e.g.,formActionormode).frontend/apps/client/src/features/discussion/ui/DiscussionForm/EnrollButton.tsx (2)
14-32: Consider renaming the event handler for consistency.The component logic is sound, but the handler name
handleClickEnrollButtondoesn't match the component nameAddButton.- const handleClickEnrollButton = () => { + const handleClickAddButton = () => { mutate(formState); };
34-52: Rename handler for consistency with component name.- const handleClickEnrollButton = () => { + const handleClickEditButton = () => { mutate(formState); };frontend/apps/client/src/features/discussion/api/mutations.ts (2)
35-54: Consistent implementation with proper cache invalidation!The edit mutation properly invalidates both list and detail queries.
- onSuccess: (_) => { + onSuccess: () => {
56-72: Well-structured delete mutation with proper cleanup!The callback execution before navigation ensures the modal is closed properly.
- onSuccess: (_) => { + onSuccess: () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 062962f8c5a38629dc08d1fd2fd789f43899658d and 7ca4ea8.
📒 Files selected for processing (9)
frontend/apps/client/src/features/discussion/api/index.ts(1 hunks)frontend/apps/client/src/features/discussion/api/mutations.ts(2 hunks)frontend/apps/client/src/features/discussion/api/queries.ts(1 hunks)frontend/apps/client/src/features/discussion/ui/DiscussionEditButton/index.tsx(1 hunks)frontend/apps/client/src/features/discussion/ui/DiscussionForm/EnrollButton.tsx(1 hunks)frontend/apps/client/src/features/discussion/ui/DiscussionForm/FormButton.tsx(1 hunks)frontend/apps/client/src/features/discussion/ui/DiscussionForm/index.tsx(1 hunks)frontend/apps/client/src/features/discussion/ui/DiscussionTitle/index.tsx(1 hunks)frontend/apps/client/src/pages/DiscussionPage/DiscussionEditPage/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/apps/client/src/features/discussion/ui/DiscussionForm/index.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#169
File: frontend/src/features/my-calendar/ui/SchedulePopover/index.tsx:35-35
Timestamp: 2025-02-14T11:44:42.393Z
Learning: Error handling for mutations will be implemented comprehensively across the application in a future update, rather than handling it individually for each mutation.
frontend/apps/client/src/pages/DiscussionPage/DiscussionEditPage/index.tsx (2)
Learnt from: hamo-o
PR: #101
File: frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverForm.tsx:10-35
Timestamp: 2025-02-06T05:14:12.328Z
Learning: Form state management and validation for the SchedulePopover component in frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverForm.tsx will be implemented in a future PR.
Learnt from: hamo-o
PR: #100
File: frontend/src/utils/jsxUtils.tsx:1-2
Timestamp: 2025-02-06T02:50:41.604Z
Learning: @tanstack/react-router package focuses on routing-specific types and does not export ReactNode type. Always import ReactNode from 'react' package to avoid type compatibility issues.
frontend/apps/client/src/features/discussion/ui/DiscussionForm/FormButton.tsx (2)
Learnt from: hamo-o
PR: #267
File: frontend/src/features/discussion/ui/DiscussionForm/MeetingPassword.tsx:16-20
Timestamp: 2025-02-21T17:55:36.208Z
Learning: In the DiscussionForm component, the password field is optional and not required for form submission.
Learnt from: hamo-o
PR: #101
File: frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverForm.tsx:10-35
Timestamp: 2025-02-06T05:14:12.328Z
Learning: Form state management and validation for the SchedulePopover component in frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverForm.tsx will be implemented in a future PR.
frontend/apps/client/src/features/discussion/api/mutations.ts (2)
Learnt from: hamo-o
PR: #169
File: frontend/src/features/my-calendar/ui/SchedulePopover/index.tsx:35-35
Timestamp: 2025-02-14T11:44:42.393Z
Learning: Error handling for mutations will be implemented comprehensively across the application in a future update, rather than handling it individually for each mutation.
Learnt from: hamo-o
PR: #100
File: frontend/src/utils/jsxUtils.tsx:1-2
Timestamp: 2025-02-06T02:50:41.604Z
Learning: @tanstack/react-router package focuses on routing-specific types and does not export ReactNode type. Always import ReactNode from 'react' package to avoid type compatibility issues.
🔇 Additional comments (12)
frontend/apps/client/src/features/discussion/api/index.ts (1)
27-37: LGTM! Clean implementation of discussion update and delete operations.The new API methods follow REST conventions and maintain consistency with the existing codebase patterns. The parameter types and endpoint structure are appropriate for the CRUD operations.
frontend/apps/client/src/features/discussion/api/queries.ts (1)
63-66: LGTM! Updated to use isPending for better alignment with React Query patterns.The change from
isLoadingtoisPendingfollows modern React Query conventions and maintains consistency with other hooks in the codebase.frontend/apps/client/src/features/discussion/ui/DiscussionEditButton/index.tsx (1)
1-26: LGTM! Well-structured conditional rendering component.The component properly handles loading states, authorization checks, and navigation. The implementation follows React best practices with appropriate conditional rendering and clean integration with the design system.
frontend/apps/client/src/features/discussion/ui/DiscussionTitle/index.tsx (2)
6-6: Good integration of the edit button component.The import of
DiscussionEditButtonproperly extends the title component with editing capabilities.
12-37: LGTM! Layout changes properly accommodate the edit button.The changes successfully:
- Update to use
isPendingfor consistency with query updates- Restructure layout to horizontal with
space-betweenfor proper button positioning- Preserve original vertical layout and styling for title/badges content
- Integrate the edit button while maintaining clean separation of concerns
frontend/apps/client/src/pages/DiscussionPage/DiscussionEditPage/index.tsx (2)
2-4: Good integration of route parameters and data fetching.The addition of
useParamsanduseDiscussionQueryproperly transforms this into a data-driven edit page.
10-40: Mapping verified: initialValues align with DiscussionRequest schema.All required fields—title, dateRangeStart, dateRangeEnd, timeRangeStart, timeRangeEnd, duration, meetingMethod, and deadline—are provided and match the zod‐inferred DiscussionRequest type. Optional fields (location, password) are correctly omitted. No mismatches found.
frontend/apps/client/src/features/discussion/ui/DiscussionForm/FormButton.tsx (1)
1-5: Clean import structure!The imports are well-organized and follow a logical separation between UI components and business logic.
frontend/apps/client/src/features/discussion/ui/DiscussionForm/EnrollButton.tsx (2)
1-13: Well-organized imports!The imports follow a logical structure with clear separation between external dependencies, internal modules, and local imports.
54-92: Excellent implementation of delete confirmation flow!The use of a modal for delete confirmation is a good UX practice, and the error styling with descriptive text properly warns users about the irreversible action.
frontend/apps/client/src/features/discussion/api/mutations.ts (2)
1-13: Comprehensive imports for mutation management!Good practice importing query keys from related features to ensure proper cache invalidation across the application.
15-33: Clean implementation of discussion creation mutation!The simplified API without callback parameter and comprehensive cache invalidation ensures data consistency across the application.
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
논의 주도자의 경우, 논의 수정과 삭제를 할 수 있도록 합니다.
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
isLoadingtoisPendingfor a smoother user experience.Refactor
Style