[FEAT] 메뉴판 삭제, 순서변경 Api 구현#77
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough메뉴판 삭제 및 인덱스(순서) 변경 API와 이를 사용하는 레포지토리/뷰모델 호출이 추가되었고, 드래그앤드롭 순서 변경 UI(리스트 상태 유틸, modifier, 화면/컴포저블) 및 삭제 확인 모달이 구현되었습니다. 네비게이션 시그니처와 일부 온보딩 네비게이션 호출도 변경됐습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MenuFolderScreen
participant MenuFolderViewModel
participant MenuFolderRepository
participant MenuFolderService
User->>MenuFolderScreen: 삭제 버튼 클릭
MenuFolderScreen->>MenuFolderScreen: DeleteMenuFolderModal 표시
User->>MenuFolderScreen: 삭제 확인
MenuFolderScreen->>MenuFolderViewModel: deleteMenuFolder(menuFolderId)
MenuFolderViewModel->>MenuFolderRepository: deleteMenuFolder(menuFolderId)
MenuFolderRepository->>MenuFolderService: deleteMenuFolder(menuFolderId)
MenuFolderService-->>MenuFolderRepository: BaseResponse<Unit>
MenuFolderRepository-->>MenuFolderViewModel: 결과 전달
MenuFolderViewModel->>MenuFolderViewModel: getMenuFolders() 호출 및 상태 갱신
User->>MenuFolderScreen: 아이템 드래그/드롭
MenuFolderScreen->>DragAndDropListState: onDragStart/onDrag/onDragInterrupted
MenuFolderScreen->>MenuFolderViewModel: updateMenuFolderList(from,to)
MenuFolderScreen->>MenuFolderViewModel: patchMenuFolders(fromId,to)
MenuFolderViewModel->>MenuFolderRepository: updateMenuFolderIndex(menuFolderId, request)
MenuFolderRepository->>MenuFolderService: updateMenuFolderIndex(menuFolderId, request)
MenuFolderService-->>MenuFolderRepository: BaseResponse<MenuFolderDetailResponse>
MenuFolderRepository-->>MenuFolderViewModel: 결과 전달
MenuFolderViewModel->>MenuFolderViewModel: getMenuFolders() 호출 및 상태 갱신
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements menu folder deletion and drag-and-drop reordering functionality for the app's menu management system.
- Adds drag-and-drop capability for reordering menu folders using long press gestures
- Implements menu folder deletion with confirmation modal
- Creates API endpoints and repository methods for delete and index update operations
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| strings.xml | Adds Korean strings for delete confirmation modal |
| dragndrop/Ext.kt | Utility functions for drag-and-drop list manipulation |
| DragAndDropListState.kt | State management class for drag-and-drop functionality |
| LandingScreen.kt | Uncomments navigation to home screen |
| MainNavHost.kt | Passes padding parameter to menu folder navigation |
| MenuFolderViewModel.kt | Adds delete and reordering logic with API calls |
| MenuFolderScreen.kt | Implements UI for drag-and-drop and deletion features |
| MenuFolderNavigation.kt | Updates navigation to pass padding parameter |
| MenuFolderButton.kt | Refactors swipe animations and adds click handlers |
| DeleteMenuFolderModal.kt | New modal component for delete confirmation |
| MenuFolderService.kt | API service methods for delete and index update |
| MenuFolderRepository.kt | Repository methods wrapping service calls |
| MenuFolderIndexRequest.kt | Data model for index update requests |
Comments suppressed due to low confidence (1)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt:82
- Large block of commented-out code should be removed to improve code readability and maintainability.
onClick = {
| fun <T> MutableList<T>.move(from: Int, to: Int) { | ||
| if (from == to) return | ||
| if (to > this.size - 1) return | ||
| Log.d("DragAndDrop", "Moving item from $from to $to , ${this.size}") |
There was a problem hiding this comment.
Debug logging should be removed from production code. Consider using a proper logging framework with different log levels or removing this debug statement.
| Log.d("DragAndDrop", "Moving item from $from to $to , ${this.size}") |
|
|
||
| val toIndex = to.coerceAtMost(_menuFolders.value.size - 1) | ||
|
|
||
| val index = _menuFolders.value[toIndex].index |
There was a problem hiding this comment.
The variable 'index' is assigned but never used in the function. This creates unnecessary computation and should be removed.
| val index = _menuFolders.value[toIndex].index |
| viewModel.patchMenuFolders( | ||
| dragStartFolderId, | ||
| dragAndDropListState.endIndex ?: 0 | ||
| ) |
There was a problem hiding this comment.
Commented-out code should be removed for better code maintenance.
| @@ -1,5 +1,7 @@ | |||
| package com.kuit.ourmenu.ui.menuFolder.navigation | |||
|
|
|||
| import android.R.attr.padding | |||
There was a problem hiding this comment.
This import appears to be unused as the parameter is of type PaddingValues from Compose, not android.R.attr.padding.
| import android.R.attr.padding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
app/src/main/java/com/kuit/ourmenu/utils/dragndrop/DragAndDropListState.kt (2)
28-29: 헤더 처리 로직인가요? 주석 추가하면 좋을 듯해요 📝
index > 0체크와-1처리가 헤더 아이템을 제외하기 위한 것 같은데, 명확성을 위해 주석을 추가하면 어떨까요?val initialIndex: Int? + // LazyList의 첫 번째 아이템(헤더)을 제외한 실제 데이터 인덱스 반환 get() = initialDraggingElement?.index?.takeIf { it > 0 }?.minus(1) ?: -1
63-93: 복잡한 로직 리팩토링 제안 🔧
onDrag메서드가 너무 복잡해 보여요. 나중에 리팩토링하실 때 아래처럼 헬퍼 메서드로 분리하면 가독성이 좋아질 것 같아요:
findOverlappingItem(): 오버랩되는 아이템 찾기shouldSwapItems(): 아이템 교체 조건 확인updateDraggedItemIndex(): 인덱스 업데이트지금은 동작하니까 괜찮지만, 나중에 시간 되실 때 고려해보세요!
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/DeleteMenuFolderModal.kt (2)
29-29: 사용하지 않는 import 정리하면 좋을 것 같아요LogoutModal import가 코드에서 사용되지 않고 있네요. 깔끔하게 제거해주세요.
-import com.kuit.ourmenu.ui.my.component.LogoutModal
85-131: 버튼 순서에 대한 UX 고려사항현재 삭제 버튼이 위에, 취소 버튼이 아래에 있는데요. 일반적인 UX 패턴에서는 위험한 액션(삭제)을 아래쪽이나 오른쪽에 배치하는 경우가 많아요. 사용자 실수를 줄이기 위한 고려사항이에요.
취소 버튼을 위로, 삭제 버튼을 아래로 변경하는 것도 고려해보세요:
- Button( - onClick = { - onConfirm() - onDismiss() - }, - modifier = Modifier - .fillMaxWidth() - .height(48.dp), - shape = RoundedCornerShape(8.dp), - colors = ButtonDefaults.buttonColors( - containerColor = Primary500Main, - contentColor = NeutralWhite - ), - ) { - Text( - text = stringResource(R.string.delete), - style = ourMenuTypography().pretendard_700_18, - color = NeutralWhite - ) - } - - Spacer(modifier = Modifier.height(8.dp)) - Button( onClick = { onDismiss() }, modifier = Modifier .fillMaxWidth() .height(48.dp), shape = RoundedCornerShape(8.dp), colors = ButtonDefaults.buttonColors( containerColor = Neutral400, contentColor = NeutralWhite ), ) { Text( text = stringResource(R.string.cancel), style = ourMenuTypography().pretendard_700_18, color = NeutralWhite ) } + + Spacer(modifier = Modifier.height(8.dp)) + + Button( + onClick = { + onConfirm() + onDismiss() + }, + modifier = Modifier + .fillMaxWidth() + .height(48.dp), + shape = RoundedCornerShape(8.dp), + colors = ButtonDefaults.buttonColors( + containerColor = Primary500Main, + contentColor = NeutralWhite + ), + ) { + Text( + text = stringResource(R.string.delete), + style = ourMenuTypography().pretendard_700_18, + color = NeutralWhite + ) + }app/src/main/java/com/kuit/ourmenu/ui/menuFolder/navigation/MenuFolderNavigation.kt (1)
3-3: 잘못된 import 수정 필요해요
android.R.attr.paddingimport는 필요없어 보여요. PaddingValues만 사용하고 있으니 이 import는 제거해주세요.-import android.R.attr.paddingapp/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (2)
95-98: 삭제 실패 시 UI 상태 복원 고려삭제 작업이 실패하면 swipedIndex가 이미 초기화되어 사용자가 혼란스러울 수 있어요. 삭제가 성공한 후에만 초기화하는 게 좋을 것 같네요.
onConfirm = { viewModel.deleteMenuFolder(deleteIndex) deleteIndex = -1 - swipedIndex = -1 }그리고 ViewModel에서 삭제 성공 시 콜백을 받아 처리하거나, 삭제 상태를 관찰해서 성공 시에만
swipedIndex = -1을 실행하는 방식을 고려해보세요.
170-170: 구현된 TODO 주석 제거드래그 앤 드롭이 이미 구현되어 있으니 이 TODO 주석은 제거해도 될 것 같아요.
-// TODO: 드래그 앤 드롭 구현app/src/main/java/com/kuit/ourmenu/ui/menuFolder/viewmodel/MenuFolderViewModel.kt (1)
60-65: 타입 일관성 개선 필요
menuFolderId를 Int로 받아서 Long으로 변환하고 있는데, 일관성을 위해 처음부터 Long으로 받는 게 좋을 것 같아요.-fun deleteMenuFolder(menuFolderId: Int) { +fun deleteMenuFolder(menuFolderId: Long) { viewModelScope.launch { _isLoading.value = true _error.value = null - menuFolderRepository.deleteMenuFolder(menuFolderId.toLong()) + menuFolderRepository.deleteMenuFolder(menuFolderId)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/MenuFolderButton.kt (3)
82-97: 주석 처리된 코드 제거사용하지 않는 주석 코드는 제거하는 게 깔끔할 것 같아요.
-// detectHorizontalDragGestures( -// onDragEnd = { -// scope.launch { -// if (offsetX < -80f) { -// onSwipe() // 다른 버튼을 닫고 이 버튼만 스와이프 -// offsetX = -maxSwipe -// } else { -// offsetX = 0f -// onReset() // 스와이프가 닫히면 상태 초기화 -// } -// } -// } -// ) { change, dragAmount -> -// change.consume() -// offsetX = (offsetX + dragAmount).coerceIn(-maxSwipe, 0f) -// }
107-107: 스와이프 임계값 상수화매직 넘버 대신 명확한 상수를 사용하면 가독성이 좋아질 것 같아요.
+private const val SWIPE_THRESHOLD_RATIO = 0.5f // ... -if (offset.value < -maxSwipe / 2) { +if (offset.value < -maxSwipe * SWIPE_THRESHOLD_RATIO) {
133-133: 주석 처리된 코드 제거사용하지 않는 offset 코드도 제거해주세요.
-// .offset(x = if (isSwiped) offset.value.dp else 0.dp)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/src/main/java/com/kuit/ourmenu/data/model/menuFolder/request/MenuFolderIndexRequest.kt(1 hunks)app/src/main/java/com/kuit/ourmenu/data/repository/MenuFolderRepository.kt(2 hunks)app/src/main/java/com/kuit/ourmenu/data/service/MenuFolderService.kt(2 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/DeleteMenuFolderModal.kt(1 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/MenuFolderButton.kt(8 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/navigation/MenuFolderNavigation.kt(3 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt(5 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/viewmodel/MenuFolderViewModel.kt(2 hunks)app/src/main/java/com/kuit/ourmenu/ui/navigator/MainNavHost.kt(1 hunks)app/src/main/java/com/kuit/ourmenu/ui/onboarding/screen/LandingScreen.kt(1 hunks)app/src/main/java/com/kuit/ourmenu/utils/dragndrop/DragAndDropListState.kt(1 hunks)app/src/main/java/com/kuit/ourmenu/utils/dragndrop/Ext.kt(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/main/java/com/kuit/ourmenu/ui/onboarding/screen/LandingScreen.kt (1)
app/src/main/java/com/kuit/ourmenu/ui/home/navigation/HomeNavigation.kt (1)
navigateToHome(11-13)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/DeleteMenuFolderModal.kt (1)
app/src/main/java/com/kuit/ourmenu/ui/theme/Type.kt (1)
ourMenuTypography(141-141)
🪛 detekt (1.23.8)
app/src/main/java/com/kuit/ourmenu/utils/dragndrop/Ext.kt
[warning] 16-16: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (15)
app/src/main/java/com/kuit/ourmenu/data/model/menuFolder/request/MenuFolderIndexRequest.kt (1)
1-8: LGTM! 깔끔한 구현이네요 👍간단한 요청 모델로 필요한 것만 잘 구현하셨네요.
app/src/main/res/values/strings.xml (1)
111-112: 문자열 리소스 추가 확인했습니다 ✅삭제 확인 모달용 문자열이 잘 추가되었네요. 사용자 친화적인 메시지입니다.
app/src/main/java/com/kuit/ourmenu/ui/navigator/MainNavHost.kt (1)
62-64: 패딩 전달 잘 추가하셨네요 ✨다른 네비게이션 그래프와 일관성 있게 padding을 전달하도록 수정하신 거 좋습니다!
app/src/main/java/com/kuit/ourmenu/ui/onboarding/screen/LandingScreen.kt (1)
66-66: 자동 홈 화면 이동이 의도된 건가요? 🏠
LaunchedEffect로 바로 홈으로 이동하는데, 이게 개발 중 테스트용인지 아니면 실제 의도된 동작인지 확인이 필요해 보여요.만약 테스트용이라면 나중에 주석 처리하거나 빌드 설정으로 분기하는 게 좋을 것 같아요!
app/src/main/java/com/kuit/ourmenu/data/repository/MenuFolderRepository.kt (3)
4-4: 새로운 import 추가 확인!MenuFolderIndexRequest import가 깔끔하게 추가되었네요.
45-49: 메뉴 폴더 삭제 함수 구현 굿!기존 패턴과 일관성 있게 runCatching과 handleBaseResponse를 사용해서 구현했네요. 에러 처리도 잘 되어 있어요.
51-59: 인덱스 업데이트 함수도 완벽!파라미터 전달과 에러 처리가 기존 코드와 동일한 패턴으로 잘 구현되어 있어요.
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/DeleteMenuFolderModal.kt (1)
37-134: 모달 구현이 깔끔하네요!Dialog를 사용한 모달 구현이 잘 되어 있고, 테마와 타이포그래피도 일관성 있게 적용되어 있어요. Preview도 잘 만들어져 있고요.
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/navigation/MenuFolderNavigation.kt (1)
37-47: 패딩 파라미터 추가 좋아요!navigation에 padding 파라미터를 추가해서 일관된 레이아웃을 지원하는 거 깔끔하네요.
app/src/main/java/com/kuit/ourmenu/data/service/MenuFolderService.kt (3)
4-4: 새로운 imports 추가 완료!필요한 import들이 깔끔하게 추가되었네요.
Also applies to: 8-11
35-38: 삭제 API 엔드포인트 굿!DELETE 방식으로 메뉴 폴더 삭제 API가 깔끔하게 정의되어 있어요. Path parameter도 적절하게 사용했고요.
40-44: 인덱스 업데이트 API도 완벽!PATCH 방식과 @Body를 사용한 요청 구조가 RESTful 하게 잘 설계되어 있네요.
app/src/main/java/com/kuit/ourmenu/utils/dragndrop/Ext.kt (1)
21-32: 드래그 모디파이어 구현 멋져요!드래그 상태에 따른 시각적 피드백(스케일링, 위치 변경, z-index)이 잘 구현되어 있네요. 사용자 경험이 좋을 것 같아요.
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (1)
51-51: 참고 링크 유효 확인
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt 51행의 주석(https://dev.to/mardsoul/how-to-create-lazycolumn-with-drag-and-drop-elements-in-jetpack-compose-part-1-4bn5)은 정상적으로 작동하며, Jetpack Compose에서 LazyColumn 드래그 앤 드롭 구현 참고 자료로 적합합니다. 주석을 그대로 유지하셔도 됩니다.app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/MenuFolderButton.kt (1)
71-75: LaunchedEffect 최적화swipe 상태가 리셋될 때 애니메이션 대신 즉시 스냅하는 건 좋은 선택이에요! 👍
|
|
||
| val toIndex = to.coerceAtMost(_menuFolders.value.size - 1) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
경계값 검증 로직 개선
빈 리스트일 때도 안전하게 처리하도록 개선이 필요해요.
+if (_menuFolders.value.isEmpty()) {
+ _error.value = "메뉴 폴더가 없습니다."
+ return@launch
+}
val toIndex = to.coerceAtMost(_menuFolders.value.size - 1)📝 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.
| val toIndex = to.coerceAtMost(_menuFolders.value.size - 1) | |
| if (_menuFolders.value.isEmpty()) { | |
| _error.value = "메뉴 폴더가 없습니다." | |
| return@launch | |
| } | |
| val toIndex = to.coerceAtMost(_menuFolders.value.size - 1) |
🤖 Prompt for AI Agents
In
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/viewmodel/MenuFolderViewModel.kt
around lines 86 to 88, the boundary check for toIndex does not safely handle the
case when the list is empty. Update the logic to first check if the list is
empty and handle that case appropriately before applying coerceAtMost, ensuring
no index out of bounds errors occur.
|
|
||
| val toIndex = to.coerceAtMost(_menuFolders.value.size - 1) | ||
|
|
||
| val index = _menuFolders.value[toIndex].index |
There was a problem hiding this comment.
사용되지 않는 변수 제거
index 변수가 계산되지만 사용되지 않고 있어요. 이 변수가 API 요청에 필요한건지 확인이 필요해요.
-val index = _menuFolders.value[toIndex].index만약 서버에 실제 index 값을 보내야 한다면:
-val index = _menuFolders.value[toIndex].index
+val targetIndex = _menuFolders.value[toIndex].index
menuFolderRepository.updateMenuFolderIndex(
fromId.toLong(),
- MenuFolderIndexRequest(toIndex)
+ MenuFolderIndexRequest(targetIndex)
)📝 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.
| val index = _menuFolders.value[toIndex].index | |
| // Replace unused `index` with `targetIndex` and pass it to the API | |
| - val index = _menuFolders.value[toIndex].index | |
| + val targetIndex = _menuFolders.value[toIndex].index | |
| - menuFolderRepository.updateMenuFolderIndex( | |
| - fromId.toLong(), | |
| - MenuFolderIndexRequest(toIndex) | |
| - ) | |
| + menuFolderRepository.updateMenuFolderIndex( | |
| + fromId.toLong(), | |
| + MenuFolderIndexRequest(targetIndex) | |
| + ) |
🤖 Prompt for AI Agents
In
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/viewmodel/MenuFolderViewModel.kt
at line 89, the variable 'index' is assigned but never used. Verify if this
'index' value is required for the API request; if not, remove the declaration to
clean up unused code. If it is needed, ensure it is properly included in the API
call or relevant logic.
| class DragAndDropListState( | ||
| val lazyListState: LazyListState, | ||
| private val onMove: (Int, Int) -> Unit | ||
| ) { |
There was a problem hiding this comment.
동시성 이슈 가능성이 있어 보여요 🤔
DragAndDropListState가 여러 mutable state를 가지고 있는데, Compose의 recomposition 중에 race condition이 발생할 수 있을 것 같아요. 특히 드래그 중에 빠른 제스처가 발생하면 문제가 될 수 있습니다.
나중에 리팩토링하실 때 @Stable 어노테이션 추가하거나 state 업데이트를 더 안전하게 처리하는 것을 고려해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/com/kuit/ourmenu/utils/dragndrop/DragAndDropListState.kt
around lines 21 to 24, the class holds multiple mutable states that may cause
race conditions during Compose recompositions, especially with rapid drag
gestures. To fix this, add the @Stable annotation to the class to inform Compose
of its stability and refactor state updates to be thread-safe and atomic,
ensuring no concurrent modifications occur during recomposition.
| fun <T> MutableList<T>.move(from: Int, to: Int) { | ||
| if (from == to) return | ||
| if (to > this.size - 1) return | ||
| Log.d("DragAndDrop", "Moving item from $from to $to , ${this.size}") | ||
| val element = this.removeAt(from) | ||
| try { | ||
| this.add(to, element) | ||
| } catch (e: IndexOutOfBoundsException) { | ||
| this.add(element) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
리스트 이동 함수에서 예외 처리 개선 필요
전반적으로 잘 구현되어 있지만, IndexOutOfBoundsException을 단순히 끝에 추가하는 방식으로 처리하면 실제 문제를 숨길 수 있어요. 로깅을 추가하거나 더 구체적인 처리를 고려해보세요.
try {
this.add(to, element)
} catch (e: IndexOutOfBoundsException) {
+ Log.w("DragAndDrop", "Index out of bounds when adding to $to, adding to end instead", e)
this.add(element)
}📝 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.
| fun <T> MutableList<T>.move(from: Int, to: Int) { | |
| if (from == to) return | |
| if (to > this.size - 1) return | |
| Log.d("DragAndDrop", "Moving item from $from to $to , ${this.size}") | |
| val element = this.removeAt(from) | |
| try { | |
| this.add(to, element) | |
| } catch (e: IndexOutOfBoundsException) { | |
| this.add(element) | |
| } | |
| } | |
| fun <T> MutableList<T>.move(from: Int, to: Int) { | |
| if (from == to) return | |
| if (to > this.size - 1) return | |
| Log.d("DragAndDrop", "Moving item from $from to $to , ${this.size}") | |
| val element = this.removeAt(from) | |
| try { | |
| this.add(to, element) | |
| } catch (e: IndexOutOfBoundsException) { | |
| Log.w("DragAndDrop", "Index out of bounds when adding to $to, adding to end instead", e) | |
| this.add(element) | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 16-16: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In app/src/main/java/com/kuit/ourmenu/utils/dragndrop/Ext.kt lines 9 to 19, the
move function catches IndexOutOfBoundsException and silently adds the element at
the end, which can hide the real issue. Improve this by adding logging inside
the catch block to record the exception details and consider rethrowing or
handling the error more explicitly to avoid masking bugs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (1)
126-128: 안전한 배열 접근 처리 잘하셨네요!이전 리뷰에서 지적된 IndexOutOfBoundsException 위험을
getOrNull로 잘 처리하셨어요. 굿!
🧹 Nitpick comments (3)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (3)
52-52: 불필요한 SuppressLint 어노테이션인 것 같아요
@SuppressLint("UnnecessaryComposedModifier")어노테이션이 있는데, 실제로 이 파일에서는Modifier.composed를 사용하지 않는 것 같네요. 튜토리얼 따라하다가 남은 코드인지 확인해보시고 필요 없다면 제거하는 게 좋을 것 같아요.
66-67: 변수명 오타와 네이밍 개선 필요해요
showDeleteModel→showDeleteModal로 수정하시고,deleteIndex는 실제로 folderId를 저장하고 있으니deleteFolderId같은 이름이 더 명확할 것 같아요.-var showDeleteModel by remember { mutableStateOf(false) } -var deleteIndex by remember { mutableIntStateOf(-1) } +var showDeleteModal by remember { mutableStateOf(false) } +var deleteFolderId by remember { mutableIntStateOf(-1) }
171-171: 오래된 TODO 주석 제거해도 될 것 같아요드래그 앤 드롭이 이미 구현되었으니 이 TODO 주석은 지워도 될 것 같네요.
-// TODO: 드래그 앤 드롭 구현
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt(5 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/viewmodel/MenuFolderViewModel.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/kuit/ourmenu/ui/menuFolder/viewmodel/MenuFolderViewModel.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (2)
app/src/main/java/com/kuit/ourmenu/utils/dragndrop/DragAndDropListState.kt (1)
rememberDragAndDropListState(13-19)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/DeleteMenuFolderModal.kt (1)
DeleteMenuFolderModal(37-134)
🔇 Additional comments (3)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (3)
54-60: PaddingValues 파라미터 추가 좋네요!네비게이션에서 시스템 바 패딩을 제대로 처리하기 위한 변경으로 보이네요. 좋은 접근입니다.
131-134: endIndex가 null일 때 0으로 처리하는 게 맞나요?
dragAndDropListState.endIndex ?: 0에서 endIndex가 null일 때 0번 위치로 이동하게 되는데, 이게 의도된 동작인지 확인이 필요해요. 드래그가 취소되거나 실패한 경우라면 API 호출을 안 하는 게 맞을 수도 있어요.-viewModel.patchMenuFolders( - dragStartFolderId, - dragAndDropListState.endIndex ?: 0 -) +dragAndDropListState.endIndex?.let { endIndex -> + viewModel.patchMenuFolders(dragStartFolderId, endIndex) +}
182-185: 삭제 기능 연동 깔끔하게 하셨네요!onDeleteClick 콜백으로 모달 표시와 삭제 대상 설정을 잘 처리하셨어요.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (1)
126-128: 인덱스 검증 로직 개선 필요이전 리뷰에서도 언급되었듯이, menuFolders가 비어있을 때 문제가 발생할 수 있어요. getOrNull을 사용하신 건 좋은데, -1을 기본값으로 사용하는 것보다 null 처리를 더 명확히 하면 좋겠네요.
-dragStartFolderId = dragAndDropListState.initialIndex?.let { index -> - menuFolders.getOrNull(index)?.menuFolderId ?: -1 -} ?: -1 +dragStartFolderId = dragAndDropListState.initialIndex?.let { index -> + menuFolders.getOrNull(index)?.menuFolderId +} ?: -1
🧹 Nitpick comments (4)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (4)
51-51: TODO 주석 대신 이슈로 관리하면 어떨까요?참고 링크를 주석으로 남기는 것도 좋지만, 코드베이스를 깔끔하게 유지하려면 이슈나 문서로 관리하는 게 나을 것 같아요.
171-171: TODO 주석 제거 필요드래그 앤 드롭이 이미 구현되었으니 이 TODO 주석은 제거해도 될 것 같네요.
-// TODO: 드래그 앤 드롭 구현
194-195: TODO 구현 또는 이슈 생성 필요메뉴판 추가 기능이 아직 구현되지 않았네요. PR 설명에서 언급하신 것처럼 나중에 리팩토링 예정이시면, 이슈를 만들어서 트래킹하면 좋을 것 같아요.
메뉴판 추가 페이지 네비게이션을 구현하는 이슈를 생성할까요?
112-120: 오버스크롤 로직 개선 제안오버스크롤 처리 로직이 복잡해 보이는데, 코루틴 Job 관리를 더 명확하게 하면 좋겠어요.
-if (overscrollJob?.isActive == true) return@detectDragGesturesAfterLongPress -dragAndDropListState - .checkOverscroll() - .takeIf { it != 0f } - ?.let { - overscrollJob = coroutineScope.launch { - dragAndDropListState.lazyListState.scrollBy(it) - } - } ?: kotlin.run { overscrollJob?.cancel() } +overscrollJob?.cancel() +val scrollAmount = dragAndDropListState.checkOverscroll() +if (scrollAmount != 0f) { + overscrollJob = coroutineScope.launch { + dragAndDropListState.lazyListState.scrollBy(scrollAmount) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/kuit/ourmenu/data/repository/MenuFolderRepository.kt(2 hunks)app/src/main/java/com/kuit/ourmenu/data/service/MenuFolderService.kt(2 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/navigation/MenuFolderNavigation.kt(3 hunks)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt(5 hunks)app/src/main/java/com/kuit/ourmenu/ui/navigator/MainNavHost.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/kuit/ourmenu/data/repository/MenuFolderRepository.kt
- app/src/main/java/com/kuit/ourmenu/ui/menuFolder/navigation/MenuFolderNavigation.kt
- app/src/main/java/com/kuit/ourmenu/ui/navigator/MainNavHost.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/main/java/com/kuit/ourmenu/ui/menuFolder/screen/MenuFolderScreen.kt (2)
app/src/main/java/com/kuit/ourmenu/utils/dragndrop/DragAndDropListState.kt (1)
rememberDragAndDropListState(13-19)app/src/main/java/com/kuit/ourmenu/ui/menuFolder/component/DeleteMenuFolderModal.kt (1)
DeleteMenuFolderModal(37-134)
🔇 Additional comments (2)
app/src/main/java/com/kuit/ourmenu/data/service/MenuFolderService.kt (2)
35-38: 삭제 API 잘 구현하셨네요!REST API 규칙에 맞게 DELETE 메서드를 사용하셨고, 경로 파라미터로 menuFolderId를 받는 구조가 깔끔합니다.
40-44: 인덱스 업데이트 API도 좋습니다!PATCH 메서드로 부분 업데이트를 구현하신 것이 RESTful하네요. 엔드포인트 경로도 명확하고 요청 본문으로 인덱스를 받는 구조가 적절합니다.
🚀 이슈번호
✏️ 변경사항
📷 스크린샷
MenuFolder_Api.mp4
✍️ 사용법
🎸 기타
코드가 많이 개판... 추후 리팩토링할 때 하겠습니다.
코드를 확실히 이해하고 진행했어야 하는데 빠르게 개발하기 위해서 ...
참고) https://dev.to/mardsoul/how-to-create-lazycolumn-with-drag-and-drop-elements-in-jetpack-compose-part-1-4bn5
Summary by CodeRabbit
신규 기능
버그 수정
UI/UX 개선
문서 및 리소스