[3단계 - 상태 관리 | 4단계 - HTTP Client] 별터 미션 제출합니다.#176
Conversation
- `CartRepository` 및 `InMemoryCartRepository`에 상품 수량 증가(`increase`) 및 감소(`decrease`) 메서드 추가 - 공통 컴포넌트 `QuantityControlButton` 구현
- 기존 `RecentItem` 관련 클래스 및 데이터베이스 테이블 명칭을 `RecentProduct`로 변경 및 마이그레이션(v2 -> v3)
- 상품 상세 화면 진입 시 `RecentProductRepository`를 통해 해당 상품을 최근 본 상품으로 저장하는 로직 추가 - UI 컴포넌트 명칭 변경 및 구조 개선: `RecentProducts` -> `RecentProductGroup`, `ShoppingBody` -> `ProductGroup`
- `RecentProductDao` 및 `RoomRecentProductRepository`의 null 안정성 보완
- `ShoppingDatabase`를 `Database`로 변경 - `ShoppingNetworkClient`를 `NetworkClient`로 변경
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📋 검토 요약이 PR은 안드로이드 쇼핑 카트 애플리케이션을 HTTP 네트워킹, Room 로컬 영속성, ViewModel 기반 상태 관리로 진화시킵니다. MockWebServer를 통한 제품 데이터 서빙, 장바구니와 최근 본 상품에 대한 Room 데이터베이스 저장, 네트워크 연결 감지 기능이 추가됩니다. 세 개 화면(Cart, ProductDetail, Shopping) 모두 Composable 로컬 상태에서 Android ViewModel 기반 아키텍처로 마이그레이션되며, 각 ViewModel은 전담 UiState 데이터 클래스를 지원합니다. 저장소 구현은 인메모리 방식에서 네트워크/Room 기반으로 전환되고, 새로운 Pager 유틸리티와 ProductUiModel로 페이지 네비게이션과 UI 모델링이 개선됩니다. 🎯 예상 코드 검토 노력🎯 4 (복잡함) | ⏱️ ~50분 ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/woowacourse/shopping/ui/cart/component/CartItemUnit.kt (1)
87-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win삭제 아이콘 설명은
닫기보다 동작에 맞는 문구가 좋겠습니다.현재 라벨이면 스크린리더가 이 액션을 잘못 안내합니다. 장바구니 항목 제거 동작과 맞는 설명으로 맞춰 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/cart/component/CartItemUnit.kt` around lines 87 - 90, The Icon's contentDescription currently reads "닫기" which misleads screen readers; update the contentDescription in CartItemUnit's Icon (the Icon with imageVector = Icons.Default.Close and modifier = Modifier.clickable(onClick = onClick)) to a verb phrase matching the action such as "장바구니 항목 삭제" or "항목 삭제" so the accessibility label correctly describes the remove-item action.
🟡 Minor comments (9)
app/src/main/java/woowacourse/shopping/model/Money.kt-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Int * Int곱셈의 오버플로우 가능성 검토 필요Line 11의
times()연산에서Int * Int는 매우 큰 수 입력 시 조용히 오버플로우됩니다. 현재 코드베이스의 가격은 8,000~15,000 수준이고 수량은 UI에서 1씩 증감되므로 실질적으로는 오버플로우가 발생하지 않을 가능성이 높습니다.그러나 Money 클래스 자체에는 명시적인 상한선이 없어, 향후 수량을 직접 설정하는 기능이 추가되거나 배치 작업이 UI를 우회하게 되면 문제가 될 수 있습니다. 현재 설계에서 가격과 수량의 제약 조건이 UI 계층에만 있는지, 아니면 데이터 모델 계층에서도 보장되어야 하는지 검토해 보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/model/Money.kt` at line 11, The times operator in Money (operator fun times(quantity: Int): Money) multiplies two Ints and can silently overflow; change the implementation to prevent silent overflow by using a wider type or explicit overflow check: either convert this.value to Long and compute Long product (e.g., val product = this.value.toLong() * quantity.toLong()) and validate against Money's allowed range before constructing a new Money, or use Math.multiplyExact(this.value, quantity) (catch ArithmeticException) and throw a clear domain error; update Money.value handling accordingly so Money.times and any constructors validate bounds and fail-fast on overflow.app/src/main/java/woowacourse/shopping/ui/component/QuantityControlButton.kt-82-83 (1)
82-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win수량 버튼의 접근성 기준 확인이 필요합니다.
현재
ControlButton의 터치 타깃 크기가 42.dp로 설정되어 있는데, Jetpack Compose 접근성 가이드에서는 클릭 가능한 컴포넌트의 최소 권장 크기가 48.dp × 48.dp입니다.프로젝트가 Material 3을 사용하고 있으므로, 다음을 검토해보세요:
- 버튼의 크기를 48.dp 이상으로 늘릴지, 아니면 터치 영역만 확장할지
- Material 3의
Modifier.minimumInteractiveComponentSize를 적용하면 레이아웃 크기는 유지하면서 터치 영역을 자동으로 최소 기준에 맞출 수 있습니다현재 코드에서
.size()다음에.clickable()이전에 이 modifier를 추가하는 방식을 고려해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/component/QuantityControlButton.kt` around lines 82 - 83, The touch target for ControlButton in QuantityControlButton (the chain that currently calls .size(42.dp).clickable(onClick = onClick)) is below the 48.dp accessibility guideline; update the modifier chain to ensure a minimum interactive size by inserting Material3's Modifier.minimumInteractiveComponentSize(48.dp) (or equivalent) before .clickable so layout size can remain 42.dp while the hit target meets 48.dp, and ensure the correct import for minimumInteractiveComponentSize is added and referenced in the QuantityControlButton/ControlButton modifier chain.app/src/main/java/woowacourse/shopping/ui/cart/CartUiState.kt-12-12 (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win빈 장바구니 상황에서 페이지 수 계산값을 의도 확인이 필요합니다.
Line 12의 식
(totalItemCount - 1) / pageSize + 1에서totalItemCount == 0이면(-1) / pageSize + 1이 되는데, Kotlin의 정수 나눗셈이 0 쪽 절단이므로 결과는 항상 1이 됩니다. 이것이 UI에서 의도한 동작인지 확인해주세요. 또한pageSize가 0 이하로 전달되는 경우를 방어할지도 함께 고려해 보시길 바랍니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/cart/CartUiState.kt` at line 12, The current totalPages getter uses (totalItemCount - 1) / pageSize + 1 which yields 1 when totalItemCount == 0 and will also misbehave if pageSize <= 0; update the totalPages implementation in CartUiState to return 0 when totalItemCount is 0 and validate pageSize > 0 (either throw an IllegalArgumentException or treat non-positive pageSize as 1) otherwise compute pages as (totalItemCount - 1) / pageSize + 1; reference the totalPages property and the totalItemCount and pageSize fields when making this change.app/src/main/java/woowacourse/shopping/ui/productdetail/component/ProductDetailBody.kt-93-93 (1)
93-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
String.format의 로케일 미지정 문제.
String.format("%,d", price)는 기본 로케일에 의존합니다(detektImplicitDefaultLocale경고). 디바이스 로케일에 따라 천 단위 구분자가 달라지거나(예: 아랍어 숫자, 유럽 일부에서.) UI 의도와 다르게 표시될 수 있습니다. 화폐 표기를 어떤 정책(앱 전역에서 항상 한국식, 또는 사용자 로케일을 따름)으로 가져갈지 결정한 뒤, 명시적인Locale을 지정해String.format(Locale.KOREA, "%,d", price)같은 형태로 호출하거나NumberFormat을 활용하는 방향을 고려해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/component/ProductDetailBody.kt` at line 93, The current formatting in ProductDetailBody.kt uses String.format("%,d", price) which relies on the implicit default Locale; update this to explicitly specify the intended locale or use NumberFormat to avoid locale-dependent separators (e.g., choose app policy: always Korean or user's locale) and replace the call that assigns formatted with an explicit-locale formatting call such as String.format(Locale.KOREA, "%,d", price) or use NumberFormat.getCurrencyInstance(...)/getNumberInstance(...) configured with the chosen Locale so the formatted string is deterministic and matches the app policy.app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailViewModel.kt-58-86 (1)
58-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
recentProductRepo.add(productId)호출 위치와 실패 처리.현재는
productRepo.findProduct(productId)가null을 반환해도(=상품 조회 실패) 그대로recentProductRepo.add(productId)를 호출해 "최근 본 상품"에 누적시킵니다. 또한try { ... } finally { ... }만 있고catch가 없어서 네트워크 오류 등 예외가 발생하면 상위로 전파되며 사용자에게는 단순히 로딩이 풀린 화면만 남게 됩니다(에러 상태/스낵바 등 노출 없음).
- "상품이 실제로 존재할 때만" 최근 상품에 추가해야 하는지(그래야
RoomRecentProductRepository에서도 dangling reference가 안 생길 텐데)- 로드 실패 시 사용자에게 어떤 피드백을 줄지 (
ProductDetailUiState에 에러 필드 도입 등)이 두 가지를 함께 정리해보면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailViewModel.kt` around lines 58 - 86, In loadProduct(), avoid always calling recentProductRepo.add(productId) and add proper error handling: call productRepo.findProduct(productId) and only call recentProductRepo.add(productId) when the returned product is non-null; wrap the repository calls in try/catch around the existing try/finally so that on exception you update _uiState to set isLoading = false and populate an error field on ProductDetailUiState (introduce an error/message property if missing) so the UI can show a snackbar or error state; ensure _uiState.update usages set selectedQuantity/lastViewedProduct only when product is non-null to prevent dangling references.app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt-31-33 (1)
31-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win잘못된 인텐트 extra에 대한 방어가 부족.
intent.getStringExtra(EXTRA_PRODUCT_ID) ?: error(...)로 누락은 막고 있지만, 값이 들어 있어도UUID.fromString(receivedProductId)가IllegalArgumentException을 던질 수 있습니다(딥링크, 잘못된 caller, 오래된 핀 단축아이콘 등 외부 진입 시나리오). 현재 흐름에서는 그대로 크래시로 이어지는데, 다음을 어떻게 처리할지 정해두면 좋겠습니다:
- 잘못된 UUID 형식일 때 단순히
finish()로 빠져나갈지UUID.fromString을runCatching으로 감싸고 사용자에게 토스트/에러 화면을 보여줄지크래시 대신 graceful failure를 한 번 정리해 보면 좋겠습니다.
Also applies to: 53-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt` around lines 31 - 33, The code currently assumes intent.getStringExtra(EXTRA_PRODUCT_ID) yields a valid UUID string (stored in receivedProductId) and lets UUID.fromString(...) crash on bad input; wrap the UUID parsing in a safe check (e.g., runCatching or try/catch around UUID.fromString(receivedProductId)) inside ProductDetailActivity, and on failure handle gracefully—show a short Toast or navigate to an error screen and call finish() instead of letting the app crash; update all usages that rely on the parsed UUID (the UUID.fromString call and any dependent logic) to use the safely-parsed result before proceeding.app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.kt-38-61 (1)
38-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win빈 결과/오류에 대한 방어 부족.
productRepo.hasNext(initialProducts.lastIndex)에서initialProducts가 비어있다면lastIndex == -1이 인자로 들어갑니다.hasNext의 계약(음수 입력에 대한 동작)이 무엇인지 한 번 짚어보고, 빈 리스트일 때는false를 직접 반환하는 안전한 처리가 더 명확하지 않을지 검토해보세요.또한 init/loadMore/increase/decrease/syncCartState 모두
try { ... } finally { ... }만 있고catch가 없습니다. 네트워크 오류 시 사용자에게는 단순히 로딩 사라짐만 보이는데,ShoppingUiState에 에러 표현을 추가해 일관된 피드백을 줄 수 있을지 정리해보면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.kt` around lines 38 - 61, Handle empty-results and surface errors to UI: when building initialProducts in init, guard productRepo.hasNext(initialProducts.lastIndex) by checking initialProducts.isEmpty and treat hasNext = false for empty lists instead of passing -1; update the _uiState copy accordingly (visibleProducts, hasNext, sizeInRepo, recentProducts). Also replace the bare try...finally blocks in init and the related functions loadMore, increase, decrease, and syncCartState with try { ... } catch (e) { _uiState.update { it.copy(error = e.message ?: "Unknown error") } } finally { _uiState.update { it.copy(isLoading = false) } } so network/other errors are captured into ShoppingUiState.error and loading is still cleared. Ensure references to productRepo.hasNext, initialProducts, mapToProductUiModels, and _uiState are used to locate the changes.app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailScreen.kt-29-48 (1)
29-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
product가 null일 때 빈 화면 노출.
uiState.product?.let { ... }외에 에러/빈 상태에 대한 분기가 없어서, 상품 로딩이 실패해product가 계속null인 경우 사용자는 빈 화면(또는 닫힌 로딩 인디케이터 뒤의 백그라운드)만 보게 됩니다.ProductDetailUiState에 에러/empty 상태를 추가해 다음과 같은 케이스에 일관된 UI를 제공하는 방향을 검토해보세요:
- 네트워크 오류로 상품을 불러오지 못한 경우 → 재시도 버튼
- 잘못된
productId로 진입한 경우 → "상품을 찾을 수 없습니다" 메시지
ProductDetailViewModel의 로드 흐름과 함께 정리하면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailScreen.kt` around lines 29 - 48, The UI currently only renders ProductDetailScreen when uiState.product is non-null and otherwise shows nothing (aside from ShoppingLoading), so update the ProductDetailUiState to include explicit loading/error/empty states (e.g., states or fields like isLoading, error: String?, isEmpty: Boolean) and change the composable rendering: inside the Box (where uiState.product is checked) add branches to render a retry error view when uiState.error is set (with a retry button that calls a new ProductDetailViewModel.retryLoad or existing loadProduct method), render a "상품을 찾을 수 없습니다" empty state when uiState.isEmpty or productId invalid, keep ShoppingLoading for uiState.isLoading, and keep ProductDetailScreen rendering when uiState.product is present; also wire ProductDetailViewModel load flow to set these new uiState fields (error/empty) appropriately on network failure or missing product.app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingScreen.kt-62-62 (1)
62-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win오프라인 안내 문구는 string resource로 분리하는 게 좋습니다.
Line [62]의 하드코딩 문자열은 다국어 대응/문구 수정 추적 시 비용이 커집니다. 리소스로 옮겨두면 유지보수가 쉬워집니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingScreen.kt` at line 62, The hardcoded Korean message in the ShoppingScreen composable should be moved to a string resource: add a new string entry (e.g., name="offline_mode_message") in your strings.xml with the text "인터넷 연결이 끊겼습니다. 오프라인 모드입니다. 😥", then replace the hardcoded Text(...) call in ShoppingScreen.kt with Text(text = stringResource(R.string.offline_mode_message)) and import androidx.compose.ui.res.stringResource where needed so localization and future edits are managed via resources.
🧹 Nitpick comments (21)
app/src/main/java/woowacourse/shopping/ui/component/ShoppingLoading.kt (1)
16-20: 💤 Low value로딩 중 상호작용 차단이 구현되었습니다.
반투명 오버레이와
clickable(enabled = false)로 터치 이벤트를 차단하고 있습니다. 현재 구현도 동작하지만, 더 확실한 이벤트 차단을 원한다면Modifier.pointerInput { /* 모든 이벤트 소비 */ }패턴을 고려해볼 수 있습니다.이 부분에 대해 더 견고한 구현을 원하시나요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/component/ShoppingLoading.kt` around lines 16 - 20, Replace the current use of Modifier.clickable(enabled = false) in ShoppingLoading's modifier with a pointerInput-based consumer to more robustly block all touch events; locate the modifier chain in ShoppingLoading.kt where modifier.fillMaxSize().background(...).clickable(...) is used and change the clickable call to Modifier.pointerInput(Unit) { awaitPointerEventScope { while (true) { awaitPointerEvent() } } } (or equivalent pointer event consumption) so all pointer events are consumed and not propagated.app/src/main/java/woowacourse/shopping/ui/shopping/component/ProductUnit.kt (1)
76-90: ⚡ Quick win여기 텍스트 스타일은 공통 토큰 재사용 여부를 다시 봐 주세요.
이 컴포넌트에서
fontSize,fontWeight,Color를 다시 하드코딩하면 이미 분리한 테마 값과 쉽게 어긋납니다. 같은 패턴이 다른 카드 컴포넌트에도 보여서, 이 지점부터라도 typography/color token으로 되돌리는 쪽이 유지보수에 유리해 보여요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/shopping/component/ProductUnit.kt` around lines 76 - 90, 두 개의 Text 컴포저블(제품명에 사용된 Text와 가격에 사용된 Text)에서 fontSize/fontWeight/Color를 하드코딩하고 있으니 ProductUnit 컴포넌트 내 해당 Text들을 앱의 기존 typography/color 토큰으로 교체하세요; 예를 들어 제품명 Text와 가격 Text에서 각각 직접 설정한 fontSize(FontWeight)와 Color를 제거하고 MaterialTheme.typography의 적절한 스타일(또는 프로젝트의 AppTypography 제품 카드용 토큰)과 MaterialTheme.colors(또는 AppColors의 토큰)를 사용하도록 바꿔 일관된 테마를 재사용하도록 수정하세요.app/src/main/java/woowacourse/shopping/network/NetworkClient.kt (3)
10-10: ⚡ Quick winOkHttpClient 인스턴스 생성 위치를 재고해보세요.
현재 각
NetworkClient인스턴스마다 새로운OkHttpClient를 생성하고 있습니다.다음을 조사해보시겠어요?
- OkHttp 문서에서 OkHttpClient를 여러 개 만드는 것과 하나를 공유하는 것의 차이는 무엇일까요?
- 연결 풀(connection pool)과 스레드 풀은 어떻게 관리되나요?
AppContainer에서NetworkClient를 어떻게 생성하고 있나요?힌트: "OkHttpClient should be shared" 또는 "OkHttpClient singleton" 같은 키워드로 찾아보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/network/NetworkClient.kt` at line 10, NetworkClient currently constructs a new OkHttpClient per instance (private val client = OkHttpClient()), which prevents sharing connection and thread pools; change this to use a single shared OkHttpClient (e.g., provide a singleton/companion object client or inject a single OkHttpClient from AppContainer) so all NetworkClient instances reuse the same connection pool and dispatcher; update NetworkClient to accept an OkHttpClient via constructor or reference a shared instance and adjust AppContainer to create/provide that singleton client.
10-10: ⚡ Quick winOkHttpClient 설정에 대해 고민해보셨나요?
현재
OkHttpClient()를 기본 설정으로 생성하고 있습니다.프로덕션 환경을 고려할 때 다음 질문들을 생각해보세요:
- 네트워크 요청이 무한정 기다리면 어떤 문제가 생길까요?
- 연결 재사용(connection pooling)은 어떻게 동작하나요?
- 디버깅이나 로깅이 필요하다면 어떻게 추가할 수 있을까요?
OkHttp 문서에서
connectTimeout,readTimeout,writeTimeout,Interceptor등의 설정 옵션을 찾아보시면 도움이 될 것 같습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/network/NetworkClient.kt` at line 10, The OkHttpClient instance in NetworkClient is created with default settings (private val client = OkHttpClient()), which risks unbounded waits and lacks logging/connection tuning; update the client construction in the NetworkClient class to configure sensible connectTimeout/readTimeout/writeTimeout values, optionally set a ConnectionPool and any needed Interceptor(s) for logging/metrics, and ensure the configured OkHttpClient instance replaces the current client field so all requests use these settings.
20-23: ⚡ Quick win에러 처리를 더 견고하게 만들 수 있을까요?
현재 코드를 살펴보면 몇 가지 시나리오가 궁금합니다:
response.body가 null일 수 있을까요? (21번 줄에서 확인하지 않음)response.body.string()이 실패하면 어떻게 될까요?- IOException 외에 발생할 수 있는 예외는 없을까요?
다음을 탐구해보세요:
- OkHttp Response의 body 속성은 nullable인가요?
- Kotlin의 null-safety를 활용해 안전하게 처리하는 방법은?
- 호출하는 쪽에서는 어떤 예외를 예상하고 처리해야 할까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/network/NetworkClient.kt` around lines 20 - 23, The network response handling must guard against a null response body and non-IO exceptions: after client.newCall(request).execute().use { response -> ... } keep the existing response.isSuccessful check, then replace direct response.body.string() with a safe pattern that uses response.body?.use { it.string() } ?: throw IOException("Empty response body") (refer to the response variable and response.body usage), and wrap the execute/use block in a try-catch that converts unexpected exceptions into an IOException (or rethrows a more specific custom exception) so callers know to expect IO-related errors while preserving the original exception as the cause.app/build.gradle.kts (1)
77-78: ⚡ Quick winMockWebServer가 프로덕션 의존성으로 포함된 이유를 검토해보세요.
MockWebServer가
testImplementation(77번 줄)과implementation(78번 줄) 양쪽에 모두 추가되어 있습니다.일반적으로 MockWebServer는 테스트 전용 라이브러리인데요. 78번 줄의
implementation스코프는:
- 프로덕션 APK에 MockWebServer를 포함시킵니다
- APK 크기를 불필요하게 증가시킬 수 있습니다
다음을 고려해보세요:
- 프로덕션 코드에서 MockWebServer를 실제로 사용하는 이유가 있나요? (예: 앱 내 목업 서버)
- 만약 테스트 목적이라면
testImplementation만으로 충분하지 않을까요?- 개발/디버그 빌드에서만 필요하다면
debugImplementation을 고려해보시겠어요?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/build.gradle.kts` around lines 77 - 78, The build includes MockWebServer twice (libs.okhttp3.mockwebserver) under both testImplementation and implementation, which unnecessarily packages a test library into production; remove the production dependency by deleting or replacing the implementation(...) entry and, depending on need, keep testImplementation(...) for tests or change the implementation(...) to debugImplementation(...) if you only need it in debug builds (update the dependency usage around the libs.okhttp3.mockwebserver symbol and ensure only testImplementation or debugImplementation remains).app/src/main/java/woowacourse/shopping/ui/cart/CartActivity.kt (2)
19-19: 💤 Low value페이지 크기가 하드코딩되어 있습니다.
pageSize = 5가 Activity에 직접 정의되어 있는데, 이 값이 변경될 가능성이나 다른 곳에서도 사용될 가능성은 없나요?다음을 고려해보세요:
- 이 값을 상수로 정의하면 어떤 장점이 있을까요?
- 여러 화면에서 같은 페이지 크기를 사용한다면 어디에 정의하는 것이 좋을까요?
- 설정이나 리소스에서 관리하는 것도 방법일 수 있을까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/cart/CartActivity.kt` at line 19, The pageSize value is hardcoded in CartActivity as 'val pageSize = 5'; move it to a named constant and reference that constant instead (e.g., define PAGE_SIZE in CartActivity's companion object or a shared constant in a paging/config file used by multiple screens). Update usages to refer to the constant (CartActivity.PAGE_SIZE or shared constant name) so the page size is configurable and discoverable; if this value must be configurable across environments consider reading it from a centralized config or resource instead of embedding the literal.
21-21: ⚖️ Poor tradeoffViewModel 생성 방식에 대해 고민해보셨나요?
현재 익명 객체로
ViewModelProvider.Factory를 구현하고 있고,@Suppress("UNCHECKED_CAST")로 경고를 억제하고 있습니다.다음 방향들을 탐색해보시겠어요?
- Jetpack Compose에서 의존성이 있는 ViewModel을 생성하는 다른 방법이 있을까요?
- 최근 Android 문서에서 권장하는 패턴은 무엇인가요? (힌트:
viewModel함수의 다른 오버로드나 최신 라이브러리 기능)- 타입 안전성을 유지하면서 캐스팅을 피할 수 있는 방법이 있을까요?
관련 키워드:
CreationExtras,ViewModelProvider.Factory, Compose ViewModel best practicesAlso applies to: 28-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/cart/CartActivity.kt` at line 21, You are suppressing an unchecked cast by creating an anonymous ViewModelProvider.Factory; replace this with a type-safe factory pattern: implement a concrete Factory that overrides create(modelClass: Class<T>, extras: CreationExtras) (or extend AbstractSavedStateViewModelFactory) to safely construct your CartViewModel without unchecked casts, remove `@Suppress`("UNCHECKED_CAST"), and then supply that factory to Compose via the viewModel(factory = ...) overload (or use hiltViewModel() if using Hilt) so you maintain type safety and follow the Compose/ViewModel best practices.app/src/main/java/woowacourse/shopping/network/NetworkMonitor.kt (2)
39-47: ⚖️ Poor tradeoff콜백 등록 순서에 대해 생각해보셨나요?
현재 코드의 실행 순서를 보면:
- 콜백을 등록합니다 (39번 줄)
- 현재 연결 상태를 확인하고 emit합니다 (41-47번 줄)
다음 시나리오를 고려해보세요:
- 콜백 등록(39줄)과 현재 상태 확인(41줄) 사이에 네트워크 상태가 변경되면 어떻게 될까요?
- 네트워크가 끊어졌다가 즉시 다시 연결되면 어떤 이벤트들이 발생할까요?
- Flow 수집자는 어떤 순서로 값을 받게 될까요?
생각해볼 점:
- 순서를 바꾸면 (현재 상태 확인 → 콜백 등록) 문제가 해결될까요, 아니면 새로운 문제가 생길까요?
- 이런 타이밍 이슈를 완전히 없앨 수 있을까요?
- 실제 사용 시나리오에서 이것이 문제가 될까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/network/NetworkMonitor.kt` around lines 39 - 47, Race between registering the NetworkCallback and emitting the current state can miss a transition; fix by: read an initial state (compute isCurrentlyConnected using connectivityManager.activeNetwork/getNetworkCapabilities), then call connectivityManager.registerNetworkCallback(request, callback), then immediately re-check the current state into latestState and use trySend to emit latestState (not the stale initial) or emit only when latestState != initial to avoid duplicate events; reference the functions/variables connectivityManager.registerNetworkCallback, isCurrentlyConnected, activeNetwork, getNetworkCapabilities, trySend, and callback to locate and update the logic.
41-46: 💤 Low valuenull-safety 처리 방식을 검토해보세요.
41-46번 줄에서
activeNetwork와NetworkCapabilities의 null 체크를 체이닝하고 있습니다.코드의 의도를 생각해보세요:
activeNetwork가 null이면 어떤 의미인가요?getNetworkCapabilities가 null을 반환하는 경우는 언제인가요?- 현재 구현에서 두 경우 모두
false로 처리하는 것이 적절한가요?Kotlin의 다른 방식들도 비교해보세요:
let,run,also등의 scope function 사용?.let { }vs?.run { }차이- 가독성과 명확성 측면에서 어떤 방식이 더 나을까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/network/NetworkMonitor.kt` around lines 41 - 46, The null-safety chaining around connectivityManager.activeNetwork and getNetworkCapabilities makes the intent unclear; update the isCurrentlyConnected computation to explicitly handle the two cases: check if connectivityManager.activeNetwork is null and treat it as no connection (optionally log/debug), then obtain val caps = connectivityManager.getNetworkCapabilities(network) and explicitly return caps?.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) == true (treating null caps as false); prefer using a clear scope like activeNetwork?.run { val caps = connectivityManager.getNetworkCapabilities(this); caps?.hasCapability(...) == true } or explicit if/return branches so the behavior for activeNetwork == null vs caps == null is obvious and maintainable (referencing connectivityManager, activeNetwork, getNetworkCapabilities, isCurrentlyConnected).app/src/test/java/woowacourse/shopping/repository/inmemory/InMemoryCartRepositoryTest.kt (1)
24-24: ⚡ Quick win테스트 커버리지 확장을 고려해보세요.
add에서increase로 API가 변경되면서, 장바구니가 수량 기반으로 동작하게 되었습니다. 현재 테스트는 단일 상품 추가만 검증하고 있는데요.다음과 같은 시나리오들도 테스트해볼 필요가 있을까요?
- 같은 상품을 여러 번
increase했을 때 수량이 올바르게 누적되는지decrease동작이 있다면 그 경우의 동작- 수량이 0이 되었을 때의 동작
PR 목표에서 언급된 "수량 컨트롤" 기능이 제대로 동작하는지 확인할 수 있는 테스트 케이스를 추가하면 어떨까요?
Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/woowacourse/shopping/repository/inmemory/InMemoryCartRepositoryTest.kt` at line 24, Expand InMemoryCartRepositoryTest to cover quantity-control scenarios: add tests that call repo.increase(product1) multiple times and assert the stored quantity increments correctly, add tests for repo.decrease(product1) to verify decrements and for the behavior when quantity reaches zero (e.g., item removed or quantity stays at 0 depending on InMemoryCartRepository contract), and assert the cart state after each operation; reference repo.increase, repo.decrease and the test class InMemoryCartRepositoryTest to locate where to add these cases.app/src/main/java/woowacourse/shopping/local/entity/CartEntity.kt (1)
7-11: 💤 Low value데이터 유효성에 대해 생각해보셨나요?
현재
CartEntity는productId와quantity를 저장하는데,quantity필드에 대한 제약사항이 없습니다.다음 시나리오들을 고려해보세요:
quantity가 음수일 수 있을까요?quantity가 0일 때의 동작은 어떻게 처리하시나요?- 최대 수량 제한이 필요할까요?
Room에서는 어떤 방식으로 이런 제약사항을 표현할 수 있을지 찾아보시겠어요? 힌트: Room의 어노테이션이나 데이터베이스 제약조건, 또는 비즈니스 로직 레이어에서의 검증 등을 고려해볼 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/local/entity/CartEntity.kt` around lines 7 - 11, CartEntity currently allows any integer for quantity; add validation to prevent invalid quantities by adding an init block in the CartEntity data class that checks the quantity (e.g., require(quantity >= 1 && quantity <= MAX_QUANTITY) { "Invalid quantity: $quantity" }), define a companion object constant MAX_QUANTITY and throw IllegalArgumentException on violation, and optionally enforce the same constraint at the DB level via a migration that adds a CHECK(quantity >= 1 AND quantity <= <MAX>) so both the CartEntity (constructor validation) and migration provide protection; reference CartEntity, quantity, productId, and MAX_QUANTITY when making the change.app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt (2)
23-25: ⚡ Quick win리포지토리 프로퍼티의 가시성 점검.
productRepo,cartRepo,recentProductRepo가public val로 노출되어 있는데, 액티비티 외부에서 이 프로퍼티들을 직접 참조해야 하는 시나리오가 따로 있을까요? 없다면private으로 좁히는 게 캡슐화 측면에서 자연스럽고, 추후 의존성 주입 방식이 바뀌어도 호출자에 미치는 영향이 줄어듭니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt` around lines 23 - 25, ProductDetailActivity에 선언된 productRepo, cartRepo, recentProductRepo 프로퍼티가 외부에서 직접 참조될 필요가 없다면 가시성을 좁혀 캡슐화를 강화하세요: ProductDetailActivity 클래스 내의 public val productRepo, cartRepo, recentProductRepo 선언을 각각 private val로 변경하고, 액티비티 외부에서 접근하는 코드(없다면 없음)를 모두 내부 접근으로 대체하거나 필요한 경우 명시적 접근자/메서드로 노출하도록 조정하세요.
38-57: 💤 Low valueFactory를 매 recomposition마다 새로 만드는 부분.
viewModel()은 이미 생성된 ViewModel을 캐시해서 반환하므로 동작 자체에는 문제가 없지만, 익명ViewModelProvider.Factory객체가 매 recomposition마다 새로 생성됩니다.remember { ... }로 감싸 한 번만 만들도록 하면 의도가 명확해지고 불필요한 객체 할당이 줄어듭니다. 동일한 패턴이 다른 화면에도 반복될 가능성이 있다면, 재사용 가능한 헬퍼/팩토리 함수로 추출하는 것도 검토해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt` around lines 38 - 57, The anonymous ViewModelProvider.Factory passed to viewModel(...) is being recreated on every recomposition; wrap its creation in remember { ... } (or extract a reusable factory helper) so the factory object is instantiated once. Locate the viewModel(...) call that constructs the anonymous object for ProductDetailViewModel (the factory that creates ProductDetailViewModel using savedStateHandle, productRepo, cartRepo, recentProductRepo and UUID.fromString(receivedProductId)) and move that object creation into a remember block (or into a shared factory function) so subsequent recompositions reuse the same Factory instance.app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailViewModel.kt (1)
30-42: 💤 Low value
increase/decrease는 동기 함수인데addToCart만 비동기인 비대칭성.
increase()/decrease()는 즉시_uiState.value를 갱신해 다음 줄에서 바로savedStateHandle[KEY_QUANTITY] = _uiState.value.selectedQuantity를 읽어 저장하는데,MutableStateFlow.update는 동기 호출이라 현재 구조에선 안전합니다. 다만 추후update람다 내부에서 추가 로직(예: 재고 검증 같은 suspend 호출)이 들어가면 자연스럽게 깨지기 쉬운 패턴이라, "선택 수량"의 진실 공급원(source of truth)을 SavedStateHandle 자체로 두거나(getStateFlow(KEY_QUANTITY, 1))update람다 내부에서 결정된 값을 곧바로 변수로 받아savedStateHandle[KEY_QUANTITY] = newValue로 쓰는 형태가 더 견고하지 않을지 검토해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailViewModel.kt` around lines 30 - 42, increase()/decrease() update _uiState and then read _uiState.value to write savedStateHandle[KEY_QUANTITY], which is fragile if update's lambda later contains extra logic; either switch to using savedStateHandle as the single source of truth (e.g., getStateFlow(KEY_QUANTITY, 1) and drive UI from that) or capture the computed newValue inside the update call (compute newQuantity, call _uiState.update { it.copy(selectedQuantity = newQuantity) }, then set savedStateHandle[KEY_QUANTITY] = newQuantity) — apply this change to the increase and decrease functions and keep KEY_QUANTITY, _uiState, savedStateHandle, increase, decrease identifiers.app/src/main/java/woowacourse/shopping/di/AppContainer.kt (2)
40-40: 💤 Low value
fallbackToDestructiveMigration(false)의도 확인.이 설정은 마이그레이션이 등록되어 있지 않으면 앱이 크래시하도록 만드는 안전한 기본값입니다(PR 본문에 v2→v3 마이그레이션이 추가되어 있다고 언급되어 있어 일관됩니다). 다만 개발/테스트 빌드에서까지 동일하게 동작하면 새로운 컬럼 실험 시 매번 앱 데이터 삭제가 필요해 번거로울 수 있으니,
BuildConfig.DEBUG등으로 빌드별 분기를 둘 가치가 있는지 한 번 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/di/AppContainer.kt` at line 40, Confirm the intended behavior of fallbackToDestructiveMigration(false) in AppContainer.kt and, if you want destructive migrations only in debug/dev builds, wrap or conditionally set fallbackToDestructiveMigration based on BuildConfig.DEBUG (or another build flag) so release builds keep the safe crash-on-missing-migration behavior while debug builds can use destructive migration to avoid data wipes during experiments; update the database builder chain where fallbackToDestructiveMigration(false) is called to apply the conditional value.
15-42: ⚡ Quick win
lateinit var database초기화 순서 위험.
cartRepository/recentProductRepository는lazy이지만, 내부에서database.cartDao()를 참조하기 때문에AppContainer.init(context)호출 전에 누군가 두 repository에 접근하면UninitializedPropertyAccessException이 즉시 발생합니다. 현재는ShoppingApplication.onCreate()에서 호출한다는 암묵적 계약에만 의존하고 있는데, 테스트/리팩터링 도중 누가 먼저 접근하는지를 정적 검증할 방법이 없습니다.다음 중 하나를 고려해 보세요:
database자체를lazy { Room.databaseBuilder(...).build() }로 만들고,Context만init(context)/별도 setter로 보관AppContainer전체를object대신 명시적 의존성 그래프로 노출(혹은 Hilt/Koin 등 DI 컨테이너 도입 검토)또한
networkClient가object초기화 시점에 즉시 생성되는데, 무거운 초기화(스레드/커넥션 풀)가 있다면 이 또한lazy로 옮기는 게 자연스러울지 짚어보면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/di/AppContainer.kt` around lines 15 - 42, cartRepository and recentProductRepository can access lateinit database before init(context) is called, risking UninitializedPropertyAccessException; make database lazy and hold a stored Context set in init(context) (e.g., store applicationContext in a private var appContext), then initialize database = lazy { Room.databaseBuilder(appContext, Database::class.java, "shopping-db").fallbackToDestructiveMigration(false).build() } so cartRepository/recentProductRepository call database.value.cartDao() safely; also change networkClient to val networkClient by lazy { NetworkClient() } to avoid heavy work at object initialization. Ensure init(context) only assigns appContext and does not build DB eagerly.app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.kt (1)
111-142: ⚡ Quick win
visibleCount와visibleProducts.size의 이중화 의도 확인.
loadMore()에서visibleCount = minOf(it.visibleCount + loadSize, totalSize)로 별도 카운터를 갱신하면서, 동시에visibleProducts도 추가된 항목으로 교체하고 있습니다.visibleProducts.size만으로 화면 표시 개수가 결정될 수 있을 텐데,visibleCount를 따로 두는 이유가 있는지 한 번 짚어보세요. 두 값이 어긋날 여지가 있고, 사용처가 중복되면 상태 관리가 복잡해집니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.kt` around lines 111 - 142, The code duplicates displayed-item tracking by maintaining visibleCount alongside visibleProducts in loadMore(); this risks divergence (visibleCount vs visibleProducts.size). Decide on a single source of truth: either remove visibleCount and derive display counts from uiState.visibleProducts.size (update all consumers of visibleCount to use visibleProducts.size), or keep visibleCount and ensure every place that mutates visibleProducts (e.g., loadMore(), any refresh/replace methods) also updates visibleCount atomically to match; update the loadMore() implementation (and any other mutators) to maintain that invariant and refactor consumers of visibleCount accordingly (look for references to visibleCount, visibleProducts, loadMore, and _uiState).app/src/main/java/woowacourse/shopping/repository/room/RoomCartRepository.kt (1)
58-67: ⚡ Quick win페이징 유효성 검사 순서와 위치 점검.
require검사가toCartItems()호출 뒤에 위치하고 있어서, 잘못된count/fromIndex가 넘어와도 일단 전체 cart entities를 읽고 (각 상품을ProductRepository로 조회한 다음) 예외를 던지게 됩니다. 또한 페이징은 Room이라면LIMIT/OFFSETSQL로 DAO에 위임하는 게 자연스럽고, 메모리 페이징은 데이터가 커질수록 부담이 됩니다.
- 인자 검사를 함수 입구로 옮겨 불필요한 I/O 전에 실패하도록 하는 방향
- DAO 쿼리에서
LIMIT :count OFFSET :fromIndex로 페이징을 처리하는 방향두 가지를 함께 고려해 보면, 입력 방어와 성능을 동시에 잡을 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/repository/room/RoomCartRepository.kt` around lines 58 - 67, The getPagedItems function performs argument validation after calling toCartItems(), causing unnecessary I/O and mapping; move the require checks (require(count >= 0) and require(fromIndex >= 0)) to the top of RoomCartRepository.getPagedItems so invalid inputs fail fast, and avoid calling toCartItems() when invalid. For performance, change the implementation to delegate paging to the DAO (e.g., add or use a Room DAO method like getPagedItems(count: Int, offset: Int) with "LIMIT :count OFFSET :offset") and have RoomCartRepository.getPagedItems call that DAO method and map results to CartItem instead of loading the entire cart via toCartItems().app/src/main/java/woowacourse/shopping/ui/productdetail/component/ProductDetailBody.kt (1)
40-80: ⚡ Quick win
Scaffold중첩 구조 개선 검토 권장
ProductDetailBody에서Scaffold를 사용하고 있는데, 이것이ProductDetailScreen의Scaffold콘텐츠 슬롯 내부에 위치하고 있습니다. Material3 공식 문서에 따르면Scaffold를 중첩할 경우 다음과 같은 문제가 발생할 수 있습니다:
- 이중 inset 적용: 외부
Scaffold의PaddingValues와 내부Scaffold의 inset 계산이 중복되어 과도한 패딩이 생길 수 있습니다.- bottomBar 충돌: 외부
CartAddButton과 내부LastViewedProductBanner의 배치/Z-order가 의도와 어긋날 수 있습니다.- 엣지-투-엣지 환경에서의 불일치: 시스템 바 inset 처리 책임이 모호해질 수 있습니다.
현재 구조에서:
- 배너를 본문의
Column내 마지막 요소로 배치하면 어떨까요?- 아니면 배너 표시 로직을
ProductDetailScreen의bottomBar로 통합하면 단일Scaffold구조가 될 텐데, 각 접근 방식이 화면 전체의 inset/Z-order 관점에서 어떻게 달라질지 생각해보세요.이런 선택지들 중 어느 것이 현재 디자인 의도에 더 부합하고 구조를 간단하게 할 수 있을지 검토해보면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productdetail/component/ProductDetailBody.kt` around lines 40 - 80, ProductDetailBody currently creates its own Scaffold (Scaffold) inside ProductDetailScreen's Scaffold which can cause double insets and bottomBar/Z-order conflicts between LastViewedProductBanner and the outer CartAddButton; remove the inner Scaffold by moving the LastViewedProductBanner out of the inner bottomBar and into the main content Column (e.g., append LastViewedProductBanner under the Column after ProductOption) or alternatively move the banner logic into ProductDetailScreen's bottomBar so only a single Scaffold wraps the screen; update ProductDetailBody to accept lastViewedProduct and onLastViewedProductClick and render the banner as a child element (or relocate the rendering call to ProductDetailScreen) and remove the inner Scaffold usage to avoid nested inset/bottomBar issues.app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingScreen.kt (1)
44-44: ⚡ Quick win
isNetworkConnected수집 방식과uiState수집 방식의 일관성을 살펴보세요.Line 42에서는
collectAsStateWithLifecycle()을 사용하고 있는데, Line 44에서는collectAsState()를 사용하고 있습니다. 동일한 ViewModel의 Flow를 수집하는 방식이 다른 이유가 있을까요?Android 공식 문서에서는 ViewModel의 StateFlow를 Jetpack Compose에서 수집할 때
collectAsStateWithLifecycle()을 사용하도록 명시하고 있습니다. 이 API는 앱이 백그라운드에 있을 때 불필요한 Flow 수집을 자동으로 일시 중지하여 배터리와 리소스를 절약합니다.isNetworkConnected도 이 패턴에 맞춰 보면 어떨까요?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingScreen.kt` at line 44, The code in ShoppingScreen collects uiState with collectAsStateWithLifecycle() but collects isNetworkConnected with collectAsState(); change the isNetworkConnected collection to use collectAsStateWithLifecycle() as well so both ViewModel Flows (viewModel.uiState and viewModel.isNetworkConnected) are collected consistently and lifecycle-aware; update the expression that binds isConnected from viewModel.isNetworkConnected to call collectAsStateWithLifecycle() (retain existing default values and variable name isConnected).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60d490e5-c03e-4c1b-90b7-2b42039071df
📒 Files selected for processing (61)
README.mdapp/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/woowacourse/shopping/ShoppingApplication.ktapp/src/main/java/woowacourse/shopping/di/AppContainer.ktapp/src/main/java/woowacourse/shopping/local/Database.ktapp/src/main/java/woowacourse/shopping/local/dao/CartDao.ktapp/src/main/java/woowacourse/shopping/local/dao/RecentProductDao.ktapp/src/main/java/woowacourse/shopping/local/entity/CartEntity.ktapp/src/main/java/woowacourse/shopping/local/entity/RecentProductEntity.ktapp/src/main/java/woowacourse/shopping/model/Cart.ktapp/src/main/java/woowacourse/shopping/model/CartItem.ktapp/src/main/java/woowacourse/shopping/model/Money.ktapp/src/main/java/woowacourse/shopping/network/NetworkClient.ktapp/src/main/java/woowacourse/shopping/network/NetworkMonitor.ktapp/src/main/java/woowacourse/shopping/repository/CartRepository.ktapp/src/main/java/woowacourse/shopping/repository/ProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/RecentProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/inmemory/InMemoryCartRepository.ktapp/src/main/java/woowacourse/shopping/repository/inmemory/InMemoryProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/network/NetworkProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/room/RoomCartRepository.ktapp/src/main/java/woowacourse/shopping/repository/room/RoomRecentProductRepository.ktapp/src/main/java/woowacourse/shopping/ui/ShoppingTypography.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartActivity.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartScreen.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartScreenState.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartUiState.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartViewModel.ktapp/src/main/java/woowacourse/shopping/ui/cart/component/CartBody.ktapp/src/main/java/woowacourse/shopping/ui/cart/component/CartHeader.ktapp/src/main/java/woowacourse/shopping/ui/cart/component/CartItemUnit.ktapp/src/main/java/woowacourse/shopping/ui/cart/component/CartPaging.ktapp/src/main/java/woowacourse/shopping/ui/component/QuantityControlButton.ktapp/src/main/java/woowacourse/shopping/ui/component/ShoppingLoading.ktapp/src/main/java/woowacourse/shopping/ui/paging/Pager.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailScreen.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailScreenState.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailUiState.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailViewModel.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/component/CartAddButton.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/component/LastViewedProductBanner.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/component/ProductDetailBody.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/component/ProductDetailHeader.ktapp/src/main/java/woowacourse/shopping/ui/shopping/ProductUiModel.ktapp/src/main/java/woowacourse/shopping/ui/shopping/ShoppingActivity.ktapp/src/main/java/woowacourse/shopping/ui/shopping/ShoppingScreen.ktapp/src/main/java/woowacourse/shopping/ui/shopping/ShoppingScreenState.ktapp/src/main/java/woowacourse/shopping/ui/shopping/ShoppingUiState.ktapp/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.ktapp/src/main/java/woowacourse/shopping/ui/shopping/component/AddButton.ktapp/src/main/java/woowacourse/shopping/ui/shopping/component/ProductGroup.ktapp/src/main/java/woowacourse/shopping/ui/shopping/component/ProductUnit.ktapp/src/main/java/woowacourse/shopping/ui/shopping/component/RecentProductGroup.ktapp/src/main/java/woowacourse/shopping/ui/shopping/component/RecentProductUnit.ktapp/src/main/java/woowacourse/shopping/ui/shopping/component/ShoppingHeader.ktapp/src/main/java/woowacourse/shopping/ui/theme/Color.ktapp/src/main/java/woowacourse/shopping/ui/theme/Type.ktapp/src/test/java/woowacourse/shopping/repository/inmemory/InMemoryCartRepositoryTest.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (4)
- app/src/main/java/woowacourse/shopping/ui/ShoppingTypography.kt
- app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailScreenState.kt
- app/src/main/java/woowacourse/shopping/ui/cart/CartScreenState.kt
- app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingScreenState.kt
| @PrimaryKey val productId: UUID, | ||
| val viewedAt: Long, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] UUID 사용 지점 확인"
rg -n --type=kt 'UUID|@Entity|@PrimaryKey' app/src/main/java/woowacourse/shopping/local app/src/main/java/woowacourse/shopping/model
echo "[2] Room TypeConverter 선언/등록 확인"
rg -n --type=kt '@TypeConverter|@TypeConverters|UUID' app/src/main/java/woowacourse/shoppingRepository: woowacourse/android-shopping-cart
Length of output: 140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] 대상 파일 확인"
find app/src/main/java/woowacourse/shopping -name "*RecentProductEntity*" -o -name "*Database*" | head -20
echo "[2] Kotlin 파일에서 UUID 및 TypeConverter 검색"
find app/src/main/java/woowacourse/shopping -name "*.kt" -exec grep -l "UUID\|@TypeConverter\|@TypeConverters" {} \;
echo "[3] 파일 구조 확인"
find app/src/main/java/woowacourse/shopping -type d | head -15Repository: woowacourse/android-shopping-cart
Length of output: 1914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== RecentProductEntity.kt ==="
cat -n app/src/main/java/woowacourse/shopping/local/entity/RecentProductEntity.kt
echo ""
echo "=== Database.kt ==="
cat -n app/src/main/java/woowacourse/shopping/local/Database.kt
echo ""
echo "=== CartEntity.kt (UUID 사용 패턴 참고) ==="
cat -n app/src/main/java/woowacourse/shopping/local/entity/CartEntity.ktRepository: woowacourse/android-shopping-cart
Length of output: 1466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 전체 `@TypeConverters` 선언 검색 ==="
find app/src/main/java/woowacourse/shopping -name "*.kt" -exec grep -n "@TypeConverters\|@TypeConverter" {} + 2>/dev/null || echo "TypeConverter 선언 없음"
echo ""
echo "=== UUID 관련 Converter 클래스 검색 ==="
find app/src/main/java/woowacourse/shopping -name "*Converter*" -o -name "*converter*"
echo ""
echo "=== build.gradle에서 Room 버전 확인 ==="
find . -name "build.gradle*" -o -name "*.gradle.kts" | xargs grep -l "room" 2>/dev/null | head -2Repository: woowacourse/android-shopping-cart
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== build.gradle.kts (Room 관련 부분) ==="
grep -n -A 2 -B 2 "room\|Room" ./app/build.gradle.kts
echo ""
echo "=== 전체 build.gradle.kts 파일 크기 ==="
wc -l ./app/build.gradle.kts
echo ""
echo "=== 전체 build.gradle.kts 내용 ==="
head -100 ./app/build.gradle.ktsRepository: woowacourse/android-shopping-cart
Length of output: 3059
UUID 타입 매핑을 위한 Room TypeConverter 등록 필요
Line 9의 UUID 타입은 Room이 기본 지원하지 않습니다. 현재 코드는 데이터베이스 작업 시 오류가 발생할 가능성이 높습니다.
프로젝트에 TypeConverter가 아직 등록되지 않았으므로 다음 단계를 따라 해결해 보세요:
- UUID를 데이터베이스 저장 가능한 형식(예: String, Long)으로 변환하는 TypeConverter 클래스를 구현해야 합니다.
- 그 후
Database클래스에@TypeConverters어노테이션으로 등록하면 됩니다.
참고로 CartEntity도 동일한 UUID 타입을 사용하고 있으니, 두 엔티티 모두 동일한 TypeConverter로 처리할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/local/entity/RecentProductEntity.kt`
around lines 9 - 10, RecentProductEntity uses a UUID field (productId: UUID)
which Room doesn't support natively; implement a Room TypeConverter class that
converts UUID to a persistable type (e.g., String) and back, register that
converter on your Room Database class using `@TypeConverters`, and reuse the same
converter for other entities that use UUID (e.g., CartEntity) so both
RecentProductEntity and CartEntity persist correctly.
| data class CartItem( | ||
| val product: Product, | ||
| val quantity: Int, | ||
| ) | ||
| ) { | ||
| val totalPrice: Money | ||
| get() = product.price * quantity |
There was a problem hiding this comment.
CartItem 수량 불변식을 생성 시점에 고정하는 게 안전합니다.
현재는 음수 quantity도 객체 생성이 가능해서, 오류가 totalPrice 계산 시점으로 늦게 드러날 수 있습니다.
CartItem 생성 시점에 수량 범위를 먼저 보장하면 디버깅이 훨씬 쉬워집니다. 혹시 음수 수량이 들어올 수 있는 입력 경로부터 먼저 추적해 보실 수 있을까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/model/CartItem.kt` around lines 3 - 8,
Add a constructor-time invariant to CartItem to prevent negative quantities: in
the CartItem class (symbols: CartItem, quantity, totalPrice) add a validation
(e.g., an init block or Kotlin require) that enforces quantity >= 0 and throws a
clear IllegalArgumentException message if violated; also audit the call sites
that instantiate CartItem (where Product and quantity are passed) to ensure
inputs cannot be negative upstream.
| override suspend fun add( | ||
| item: Product, | ||
| quantity: Int, | ||
| ) { | ||
| val existingIndex = value.indexOfFirst { it.product.id == item.id } | ||
|
|
||
| if (existingIndex != -1) { | ||
| val existingItem = items[existingIndex] | ||
| items[existingIndex] = existingItem.copy(quantity = existingItem.quantity + 1) | ||
| val existingItem = value[existingIndex] | ||
| value[existingIndex] = existingItem.copy(quantity = quantity) | ||
| } else { | ||
| items.add(CartItem(product = item, quantity = 1)) | ||
| value.add(CartItem(product = item, quantity = quantity)) | ||
| } | ||
| } |
There was a problem hiding this comment.
add()에서 0 이하 수량을 막아두는 게 좋겠습니다.
지금은 quantity가 0이나 음수여도 그대로 저장돼서 cart invariant가 깨집니다. 여기서 quantity > 0를 강제할지, 0은 삭제로 정규화할지 한 방향으로 먼저 고정해 두면 이후 UI/DB 로직이 훨씬 단순해집니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/woowacourse/shopping/repository/inmemory/InMemoryCartRepository.kt`
around lines 15 - 27, The add(...) method currently allows quantity <= 0 which
breaks cart invariants; update InMemoryCartRepository.add to normalize
non‑positive quantities by treating them as removal: at the start of add(Product
item, Int quantity) check if quantity <= 0 and if so remove any existing
CartItem for that product from the backing collection (value) and return;
otherwise proceed with the existing upsert logic (finding existingIndex, copying
or adding a CartItem). This keeps CartItem quantity always > 0 and centralizes
removal behavior in add.
| override suspend fun decrease(item: Product) { | ||
| val existingIndex = value.indexOfFirst { it.product.id == item.id } | ||
| require(existingIndex != -1) { "장바구니에 해당 제품(${item.name})이 없습니다." } | ||
|
|
||
| val cartItem = value[existingIndex] | ||
|
|
||
| if (cartItem.quantity > 1) { | ||
| items[index] = cartItem.copy(quantity = cartItem.quantity - 1) | ||
| value[existingIndex] = cartItem.copy(quantity = cartItem.quantity - 1) | ||
| } else { | ||
| items.removeAt(index) | ||
| value.removeAt(existingIndex) | ||
| } | ||
| } | ||
|
|
||
| override suspend fun delete(item: Product) { | ||
| val existingIndex = value.indexOfFirst { it.product.id == item.id } | ||
| require(existingIndex != -1) { "장바구니에 해당 제품(${item.name})이 없습니다." } | ||
|
|
||
| value.removeAt(existingIndex) | ||
| } |
There was a problem hiding this comment.
없는 상품에 대한 decrease/delete는 예외보다 멱등 처리 쪽이 안전합니다.
화면 상태가 늦게 갱신되거나 버튼이 연속 탭되면 이미 제거된 상품에 대한 호출이 쉽게 들어올 수 있는데, 지금은 require로 앱이 바로 터집니다. 여기서는 no-op로 무시할지, 호출자에게 실패 결과를 돌려줄지 한 번 정리해 두는 편이 안정적입니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/woowacourse/shopping/repository/inmemory/InMemoryCartRepository.kt`
around lines 40 - 58, The decrease and delete implementations in
InMemoryCartRepository currently throw via require when the product isn't
present; make them idempotent by treating missing items as a no-op (or
alternatively return a failure result if you prefer) so repeated/late calls
don't crash the app: in the decrease(item: Product) and delete(item: Product)
methods, replace the require(existingIndex != -1) check with an early return
when index == -1, and keep the existing removal/quantity-update logic otherwise;
ensure you reference the existingIndex, cartItem, and value list variables as in
the current functions so behavior and concurrency remain unchanged.
| override suspend fun add( | ||
| item: Product, | ||
| quantity: Int, | ||
| ) { | ||
| val currentEntity = cartDao.getCartItemById(item.id) | ||
| if (currentEntity != null) { | ||
| cartDao.updateQuantity(item.id, currentEntity.quantity + quantity) | ||
| } else { | ||
| cartDao.insert(CartEntity(item.id, quantity)) | ||
| } | ||
| } | ||
|
|
||
| override suspend fun increase(item: Product) { | ||
| val currentEntity = cartDao.getCartItemById(item.id) | ||
|
|
||
| if (currentEntity != null) { | ||
| cartDao.updateQuantity(productId = item.id, currentEntity.quantity + 1) | ||
| } else { | ||
| cartDao.insert(CartEntity(item.id, 1)) | ||
| } | ||
| } | ||
|
|
||
| override suspend fun decrease(item: Product) { | ||
| val currentEntity = cartDao.getCartItemById(item.id) | ||
|
|
||
| if (currentEntity != null) { | ||
| if (currentEntity.quantity > 1) { | ||
| cartDao.updateQuantity(item.id, currentEntity.quantity - 1) | ||
| } else { | ||
| cartDao.deleteById(item.id) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Room Kotlin: @transaction DAO method vs db.withTransaction for read-modify-write upsert
💡 Result:
For read-modify-write upsert operations in Room Kotlin, use @Transaction on a DAO method containing the read (query), modify, and upsert logic. This ensures atomicity, preventing lost updates from concurrent transactions [1][2][3]. Example: kotlin `@Dao` interface UserDao { `@Query`("SELECT * FROM users WHERE id = :id") suspend fun getUser(id: Long): User? `@Upsert` suspend fun upsertUser(user: User) `@Transaction` open suspend fun upsertUserAtomic(id: Long, newData: UserData): User { val existing = getUser(id) val toUpsert = existing?.copy(...) ?: newData.toUser(id) // modify logic upsertUser(toUpsert) return toUpsert } } @Transaction wraps the entire method in one transaction; note functions must be open in abstract DAOs [1][2]. Individual @Insert/@Upsert/@update are already transactional but separate calls risk races [1][4]. Alternatively, use db.withTransaction { } in repositories for multi-DAO ops or flexibility [5][6][3]: kotlin suspend fun upsertUserAtomic(db: AppDatabase, id: Long, newData: UserData) { db.withTransaction { val user = db.userDao().getUser(id) val toUpsert = // modify logic db.userDao().upsertUser(toUpsert) } } Both prevent read-modify-write races by serializing operations [2][7]. Prefer @Transaction for single-DAO encapsulation; withTransaction for repository-level control [5]. @Upsert (Room 2.5+) is atomic itself for simple insert-or-update without read [7][4].
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/room3/Transaction
- 2: https://commonsware.com/Room/pages/chap-dao-005.html
- 3: https://developer.android.com/reference/kotlin/androidx/room/RoomDatabase
- 4: https://developer.android.com/reference/kotlin/androidx/room/Upsert
- 5: https://stackoverflow.com/questions/67569328/kotlin-room-withtransaction-in-repository-or-transaction-in-dao
- 6: https://developer.android.com/reference/androidx/room3/RoomDatabaseKt
- 7: https://medium.com/@doanthanhtungnk123/room-in-depth-the-ultimate-guide-to-embedded-transactions-upsert-indexes-relations-and-8d11e9317ca6
장바구니 추가/증가/감소 작업에서 동시성 경합(race condition) 처리 필요.
add, increase, decrease 메서드는 getCartItemById로 현재 상태를 조회한 후 조건에 따라 updateQuantity, insert, deleteById를 호출합니다. 두 개 이상의 코루틴이 같은 상품에 대해 동시에 이 메서드들을 실행하면, 둘 다 같은 currentEntity를 읽은 뒤 각각 쓰기를 시도하면서 한쪽 갱신이 손실되거나 제약 조건 충돌(UNIQUE constraint violation)이 발생할 수 있습니다.
Room에서는 이런 읽기-수정-쓰기 패턴의 원자성을 보장하기 위해 다음 방법 중 하나를 사용합니다:
- DAO 메서드에
@Transaction추가: 조회와 쓰기를 한 트랜잭션으로 묶기 @Upsert활용 (Room 2.5+): 조건 검사 없이 삽입/갱신을 원자적으로 처리하는 단순 경우에 유용db.withTransaction { }블록: Repository 레벨에서 여러 DAO 호출을 트랜잭션으로 감싸기
장바구니는 빠른 연타 입력과 백그라운드 동기화가 겹칠 수 있는 경로인데, 이 메서드들이 동시에 호출될 가능성을 감안해 어떤 방식이 현재 구조에 가장 적합한지 검토해보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/repository/room/RoomCartRepository.kt`
around lines 20 - 52, The add, increase, and decrease methods perform
read-then-write sequences that can race; make each operation atomic by executing
the read and subsequent update/insert/delete inside a single Room transaction.
Modify the implementation of add, increase, and decrease (in RoomCartRepository)
to call the database transaction API (e.g., db.withTransaction { ... }) or move
the logic into a DAO method annotated with `@Transaction` (or use `@Upsert` for
simple insert-or-update cases), wrapping the getCartItemById / updateQuantity /
insert / deleteById calls so the entire read-modify-write is executed atomically
for the given product id (reference: add, increase, decrease,
cartDao.getCartItemById, cartDao.updateQuantity, cartDao.insert,
cartDao.deleteById, CartEntity).
| fun addToCart() { | ||
| val currentState = _uiState.value | ||
| val productToSave = currentState.product ?: return | ||
|
|
||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } | ||
| try { | ||
| cartRepo.add(productToSave, quantity = currentState.selectedQuantity) | ||
| } finally { | ||
| _uiState.update { it.copy(isLoading = false) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
selectedQuantity 초기화와 addToCart의 의미가 어긋날 가능성.
loadProduct()에서 selectedQuantity를 "장바구니에 이미 담긴 수량"으로 초기화하고 있는데, addToCart()는 cartRepo.add(productToSave, selectedQuantity)로 호출되며, RoomCartRepository.add는 기존 수량에 quantity를 더하는 방식입니다.
예시: 사용자가 이미 장바구니에 3개 담은 상품을 상세 화면에서 다시 열면 selectedQuantity = 3이 됩니다. 이때 사용자가 "+" 한 번 눌러 화면에 4가 표시된 뒤 "장바구니 담기"를 누르면, 장바구니 수량은 3 + 4 = 7이 됩니다(사용자 입장에서는 "4개로 만들기" 의도였을 텐데 결과가 7).
다음 중 어느 쪽이 의도된 동작인지 정리해보세요:
- 상세 화면의
selectedQuantity는 "이번에 추가로 담을 수량"이며 항상 1부터 시작 → 초기화 로직 수정 - 상세 화면의
selectedQuantity는 "장바구니의 최종 수량" →CartRepository에 "set" 시맨틱이 필요(혹은 add 대신 absolute 업데이트)
이 부분이 사용자 시나리오에서 가장 잘 드러나는 곳이니 한 번 짚어보고 가는 게 좋겠습니다.
Also applies to: 58-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailViewModel.kt`
around lines 44 - 56, The current mismatch is that loadProduct() initializes
selectedQuantity to the existing cart amount while addToCart() calls
cartRepo.add(product, selectedQuantity) and RoomCartRepository.add increments
existing quantity, causing double-add semantics; pick one consistent behavior
and implement it: either (A) make the detail view's selectedQuantity represent
"additional to add" by changing loadProduct()/initialization of selectedQuantity
to 1 (and keep using cartRepo.add in addToCart), or (B) make selectedQuantity
represent the desired final cart quantity by changing addToCart() to call a
"set" semantics method on the repository (e.g.,
cartRepo.setQuantity/updateQuantity) or modify RoomCartRepository.add to accept
a replace/set flag; update references to selectedQuantity, loadProduct(),
addToCart(), cartRepo.add and RoomCartRepository.add accordingly so behavior
matches the chosen intent.
| Icon( | ||
| imageVector = Icons.Default.ShoppingCart, | ||
| contentDescription = "장바구니", | ||
| modifier = Modifier.clickable(onClick = onCartClick), | ||
| tint = Color.White, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Material Design 3 및 Jetpack Compose 공식 가이드에서 권장하는 최소 터치 타깃 크기(48dp 관련)를 확인하고, Icon 클릭 영역 설계 권장 패턴을 알려줘.
💡 Result:
Material Design 3과 Jetpack Compose 공식 가이드에서 최소 터치 타깃 크기는 48dp x 48dp로 권장됩니다[1][2][3][4]. 이는 접근성 지침을 따르며, 아이콘(보통 24dp)은 패딩으로 둘러싸여 전체 48dp 영역을 확보합니다[2][5][6]. 시스템은 작은 clickable 영역을 자동 확장하지만, minimumInteractiveComponentSize modifier를 사용해 레이아웃에서 48dp를 예약하는 것이 권장됩니다[1][4]. Icon 클릭 영역 설계 패턴: IconButton 컴포저블을 사용하세요. IconButton은 내부적으로 48dp 최소 터치 타깃을 보장하며, Icon(24dp)을 중앙에 배치하고 패딩을 추가합니다[5][6][7]. 예시 코드: kotlin IconButton( onClick = { /* 클릭 액션 */ }, modifier = Modifier.minimumInteractiveComponentSize() // 필요시 명시적 적용 ) { Icon( imageVector = Icons.Default.Favorite, contentDescription = "Favorite" ) } Material 3 IconButton은 FilledIconButton, OutlinedIconButton 등 변형을 지원하며, 접근성을 위해 contentDescription을 반드시 제공하세요[7]. 터치 타깃 간 8dp 이상 간격을 유지하세요[2].
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/compose/material3/minimumInteractiveComponentSize.modifier
- 2: https://support.google.com/accessibility/android/answer/7101858
- 3: https://android.googlesource.com/platform/frameworks/support/+/bea20f7f4c74346e7dfc5a2b800ba0e4307e5602/compose/material3/material3-common/src/commonMain/kotlin/androidx/compose/material3/common/InteractiveComponentSize.kt
- 4: https://developer.android.com/develop/ui/compose/accessibility/api-defaults
- 5: https://kotlinlang.org/api/compose-multiplatform/material3/androidx.compose.material3/-icon-button.html
- 6: https://developer.android.com/reference/kotlin/androidx/compose/material/IconButton.composable
- 7: https://developer.android.com/develop/ui/compose/components/icon-button
쇼핑 카트 아이콘의 터치 영역을 Material Design 표준에 맞춰 확장하세요.
현재 코드는 Icon(24dp)에 clickable 수정자만 적용했는데, Material Design 3과 Jetpack Compose 공식 지침에서는 최소 48dp × 48dp의 터치 타깃을 권장합니다.
48dp 기준을 충족하려면 어떤 방식으로 접근할 수 있을까요? 이 경우 IconButton 컴포저블 사용을 검토해보시면 좋을 것 같습니다. IconButton은 내부적으로 48dp 최소 터치 타깃을 보장하면서도 Icon을 중앙에 배치하는 패턴이 표준 방식입니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/woowacourse/shopping/ui/shopping/component/ShoppingHeader.kt`
around lines 60 - 64, Replace the direct Icon clickable usage with a Material
IconButton wrapper so the touch target meets the 48dp minimum; specifically, in
ShoppingHeader.kt wrap the Icon(...) that currently uses modifier =
Modifier.clickable(onClick = onCartClick) with an IconButton(onClick =
onCartClick) and move the Icon inside (keeping imageVector =
Icons.Default.ShoppingCart, contentDescription = "장바구니", tint = Color.White) so
the IconButton enforces the proper touch size and centers the icon.
| fun increase(product: Product) { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } | ||
| try { | ||
| cartRepo.increase(product) | ||
|
|
||
| _uiState.update { state -> | ||
| val updatedProducts = | ||
| state.visibleProducts.map { uiModel -> | ||
| if (uiModel.product.id == product.id) { | ||
| uiModel.copy(cartQuantity = uiModel.cartQuantity + 1) | ||
| } else { | ||
| uiModel | ||
| } | ||
| } | ||
| val cartCount = state.cartCount + 1 | ||
| state.copy(visibleProducts = updatedProducts, cartCount = cartCount) | ||
| } | ||
| } finally { | ||
| _uiState.update { it.copy(isLoading = false) } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun decrease(product: Product) { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } | ||
| try { | ||
| cartRepo.decrease(product) | ||
|
|
||
| _uiState.update { state -> | ||
| val updatedProducts = | ||
| state.visibleProducts.map { uiModel -> | ||
| if (uiModel.product.id == product.id) { | ||
| uiModel.copy(cartQuantity = maxOf(0, uiModel.cartQuantity - 1)) | ||
| } else { | ||
| uiModel | ||
| } | ||
| } | ||
| val cartCount = maxOf(0, state.cartCount - 1) | ||
| state.copy(visibleProducts = updatedProducts, cartCount = cartCount) | ||
| } | ||
| } finally { | ||
| _uiState.update { it.copy(isLoading = false) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
increase/decrease에서 전체 로딩을 띄우는 UX 검토.
현재 구조에서는 사용자가 상품 카드의 "+" 또는 "−" 버튼을 누를 때마다 _uiState.update { it.copy(isLoading = true) }로 전체 화면 로딩을 띄우고 작업이 끝나면 풀어줍니다. 빠른 연타 시 매 입력마다 로딩 UI가 깜빡일 수 있고, 리스트 스크롤이나 다른 인터랙션이 잠시 막힐 수 있습니다.
수량 조정 같은 가벼운 작업은 보통 낙관적(optimistic) 업데이트로 처리하고 로딩 인디케이터는 노출하지 않는 편이 자연스럽습니다. 실패 시 롤백 전략까지 함께 정리해보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.kt`
around lines 63 - 109, Replace the full-screen loading behavior in increase and
decrease by performing optimistic updates: remove the initial and final
_uiState.update { it.copy(isLoading = true/false) } calls in increase and
decrease, immediately update visibleProducts and cartCount in the UI
optimistically inside the existing _uiState.update block, then call
cartRepo.increase(product) / cartRepo.decrease(product) asynchronously (still
inside viewModelScope.launch) and on failure revert the specific product's
cartQuantity and cartCount back to their previous values and emit an error
state; keep using the existing viewModelScope.launch, _uiState, and the
visibleProducts mapping logic but add a try/catch around the repo call to
rollback and surface errors instead of toggling global isLoading.
| fun decrease(product: Product) { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } | ||
| try { | ||
| cartRepo.decrease(product) | ||
|
|
||
| _uiState.update { state -> | ||
| val updatedProducts = | ||
| state.visibleProducts.map { uiModel -> | ||
| if (uiModel.product.id == product.id) { | ||
| uiModel.copy(cartQuantity = maxOf(0, uiModel.cartQuantity - 1)) | ||
| } else { | ||
| uiModel | ||
| } | ||
| } | ||
| val cartCount = maxOf(0, state.cartCount - 1) | ||
| state.copy(visibleProducts = updatedProducts, cartCount = cartCount) | ||
| } | ||
| } finally { | ||
| _uiState.update { it.copy(isLoading = false) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
decrease 시 cartCount 갱신이 실제 cart 상태와 어긋날 수 있음.
RoomCartRepository.decrease는 해당 상품이 장바구니에 없거나 수량이 이미 0인 경우 아무 동작도 하지 않는데, 여기서는 cartRepo.decrease(product) 성공 여부와 무관하게 cartCount = maxOf(0, state.cartCount - 1)로 한 칸씩 깎습니다. 결과적으로 visibleProducts의 cartQuantity 합계와 cartCount가 어긋날 수 있습니다(예: 이미 0인 상품에 대해 "-"가 호출된 경우).
또한 increase도 같은 맥락에서 "실제 cart 변경량"이 아니라 "+1"로 가정해 합산하고 있습니다. cart의 실제 합계는 cartRepo 호출 결과(또는 직후 재조회)에서 도출해 동기화하는 게 desync 위험을 줄여줍니다. 어느 시점에 진실 공급원을 cart로 둘지 정리해보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.kt`
around lines 87 - 109, The decrease/increase handlers assume the repo always
changes the cart and unconditionally +/-1 cartCount, causing desync when
RoomCartRepository.decrease/increase are no-ops; update these methods to return
an explicit result (e.g., boolean success or the updated cart) or immediately
re-fetch the cart after the repo call, then use that authoritative result to
compute cartCount and each visibleProducts' cartQuantity before updating
_uiState in decrease (and mirror the same change in increase); in short, rely on
cartRepo's returned outcome (or a fresh cart query) to derive cartCount and
visibleProducts rather than naïvely applying ±1 in
ShoppingViewModel.decrease/increase.
| fun syncCartState() { | ||
| viewModelScope.launch { | ||
| val cartItems = cartRepo.getAllCartItems() | ||
| val totalCartCount = cartItems.items.sumOf { it.quantity } | ||
| val recentProducts = recentProductRepo.getRecentProducts() | ||
|
|
||
| _uiState.update { state -> | ||
| val currentProducts = state.visibleProducts.map { it.product } | ||
| val updatedUiModels = mapToProductUiModels(currentProducts) | ||
|
|
||
| state.copy( | ||
| visibleProducts = updatedUiModels, | ||
| cartCount = totalCartCount, | ||
| recentProducts = recentProducts, | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
syncCartState에서 sizeInRepo/hasNext 갱신 누락과 mapToProductUiModels의 비용.
syncCartState()는 cart/recentProducts는 다시 가져오지만sizeInRepo와hasNext는 갱신하지 않습니다. 다른 화면에서 새 상품이 추가되거나 줄어든 경우(또는 네트워크 복구 등)loadMore조건이 어긋날 수 있는데 의도된 동작인지 확인해 주세요.mapToProductUiModels는 매번cartRepo.getAllCartItems()를 호출하는데,RoomCartRepository.getAllCartItems()내부는 cart의 각 row마다productRepository.findProduct(...)를 호출합니다(NetworkProductRepository기준 네트워크 N회).init,loadMore,syncCartState에서 모두 매번 호출되니, cart의 수량 맵만 따로 가벼운 경로(예: 새 DAO 쿼리)로 빼는 방향을 한 번 고려해보세요.
Also applies to: 163-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/ui/shopping/ShoppingViewModel.kt`
around lines 144 - 161, syncCartState currently refreshes cart items and
recentProducts but doesn't update pagination flags (sizeInRepo/hasNext) and
mapToProductUiModels is expensive because it calls cartRepo.getAllCartItems()
which triggers per-row product lookups; update syncCartState (and the similar
block at 163-175) to compute and set sizeInRepo and hasNext on the UI state (use
cartRepo size/count or items.size and compare with pageSize logic used by
loadMore/init) and refactor mapToProductUiModels to accept a lightweight
quantity map (e.g., Map<productId, quantity>) instead of calling
cartRepo.getAllCartItems() itself — add a new efficient cartRepo method (or DAO
query) like getProductQuantityMap()/getCartSize() used by
init/loadMore/syncCartState to avoid N product lookups and pass that map into
mapToProductUiModels for composing visibleProducts.
- `add` 메서드 호출 시 수량이 1 이상인지 검증하는 로직 추가 - `decrease` 메서드에서 대상 상품이 없는 경우 예외 발생 대신 조기 종료하도록 수정
- `CartDao` 내 페이징(`LIMIT`, `OFFSET`) 및 전체 개수(`COUNT`) 조회를 위한 메서드 추가 - 메모리에서 리스트 전체를 불러와 처리하던 페이징 및 사이즈 확인 로직을 DB 레벨에서 처리하도록 개선 - 장바구니 아이템 조회 시 매핑되지 않는 유효하지 않은 상품을 DB에서 삭제하는 로직 추가
SeongHoonC
left a comment
SeongHoonC
left a comment
There was a problem hiding this comment.
안녕하세요 별터!
3, 4 단계 새로운 키워드가 많아 어려웠을텐데 잘해주셨어요!
내용이 많은만큼 중요하다고 생각하는 부분에 리뷰를 달았습니다.
확인하시다가 궁금한 점이나 같이 고민해보고 싶은 부분 있으면 언제든 DM이나 코멘트 주세요!
끝까지 화이팅입니다 🙌
| fun init(context: Context) { | ||
| database = | ||
| Room | ||
| .databaseBuilder( | ||
| context.applicationContext, | ||
| Database::class.java, | ||
| "shopping-db", | ||
| ).fallbackToDestructiveMigration(false) | ||
| .build() | ||
| } |
There was a problem hiding this comment.
object init 호출과 각 프로퍼티 참조 시점이 뒤바뀔 수 있을 것 같아요. class 로 변경하고 database 가 build 된 상태에서 참조하는 것이 보장되도록 변경해보는건 어떤가요?
| @Insert(onConflict = OnConflictStrategy.IGNORE) | ||
| suspend fun insert(cartEntity: CartEntity) | ||
|
|
||
| @Query("UPDATE cart_items SET quantity = :quantity WHERE productId = :productId") | ||
| suspend fun updateQuantity( | ||
| productId: UUID, | ||
| quantity: Int, | ||
| ) |
There was a problem hiding this comment.
동일한 id 상품은 담지 않는 요구사항은 잘 만족하고 있으나 사용하는 곳을 보면 없으면 추가하고 있으면 업데이트하는 로직이 반복되고 있어요.
primary key 가 없으면 추가하고 있으면 업데이트하는 방식으로 변경하는건 어떤가요?
upsert 나 OnConflictStrategy.REPLACE 같은게 있겠군요
| override suspend fun add( | ||
| item: Product, | ||
| quantity: Int, | ||
| ) { | ||
| val currentEntity = cartDao.getCartItemById(item.id) | ||
| if (currentEntity != null) { | ||
| cartDao.updateQuantity(item.id, quantity) | ||
| } else { | ||
| cartDao.insert(CartEntity(item.id, quantity)) | ||
| } | ||
| } |
There was a problem hiding this comment.
함수 이름은 add 이지만 이미 Id 가 존재하면 덮어쓰고 있습니다. 사용부에서 오용하지 않도록 의도를 명확히 드러내주세요
| DisposableEffect(lifecycleOwner) { | ||
| val observer = | ||
| LifecycleEventObserver { _, event -> | ||
| if (event == Lifecycle.Event.ON_RESUME) { | ||
| viewModel.syncCartState() | ||
| } | ||
| } | ||
|
|
||
| lifecycleOwner.lifecycle.addObserver(observer) | ||
| onDispose { | ||
| lifecycleOwner.lifecycle.removeObserver(observer) | ||
| } | ||
| } |
There was a problem hiding this comment.
LifecycleOwner 의 확장함수 repeatOnLifecycle 를 인지하고 의도적으로 DisposableEffect 를 사용하신건지 궁금해요!
|
|
||
| fun increase(product: Product) { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } |
There was a problem hiding this comment.
update 와 setValue 차이는 무엇이고 update 를 사용한 이유는 무엇인가요?
_uiState.update { ... }
_uiState.value = ...
| val state by viewModel.uiState.collectAsStateWithLifecycle() | ||
| val lifecycleOwner = LocalLifecycleOwner.current | ||
| val isConnected by viewModel.isNetworkConnected.collectAsState() |
There was a problem hiding this comment.
상태와 달리 네트워크 상태는 Lifecycle 에 의존하지 않고 있어요. 이렇게 의도하신 이유가 궁금합니다!
| fun addToCart() { | ||
| val currentState = _uiState.value | ||
| val productToSave = currentState.product ?: return | ||
|
|
||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } | ||
| try { | ||
| cartRepo.add(productToSave, quantity = currentState.selectedQuantity) | ||
| } finally { | ||
| _uiState.update { it.copy(isLoading = false) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
상품 상세에서 선택한 상품의 개수만큼 장바구니에 추가되어야하는데 지금은 변경하고 있습니다. B마트 UX 를 확인하고 동일하게 수정해주세요.
| onAddToCartClick = { | ||
| state.addToCart(product) | ||
| viewModel.addToCart() | ||
| onAddToCartClick() | ||
| }, |
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } | ||
| try { | ||
| val product = productRepo.findProduct(productId) |
There was a problem hiding this comment.
product 가 null 이어도 아래 작업들이 수행되고 있어요.
| object : ViewModelProvider.Factory { | ||
| override fun <T : ViewModel> create(modelClass: Class<T>): T = | ||
| CartViewModel( | ||
| cartRepo = cartRepo, | ||
| pageSize = pageSize, | ||
| ) as T | ||
| }, |
There was a problem hiding this comment.
ViewModelFactory 자체는 CartViewModel 를 생성하는 방법이기 때문에 화면에서 직접 만드는 것 보다 분리하는게 어떤가요?
…트 UX 방식으로 수정) - `ProductDetailViewModel`에서 `SavedStateHandle`을 통한 수량 보존 로직 및 초기 진입 시 장바구니 수량 동기화 로직 제거 - 상품 상세 페이지 진입 시 선택 수량이 항상 1로 초기화되도록 변경 - `RoomCartRepository`에서 기존 상품이 장바구니에 있을 경우, 입력된 수량으로 덮어쓰는 대신 기존 수량에 더하도록 수정
2 participants
🚀 쇼핑 장바구니 - 상태 관리, HTTP Client
Background
+ 버튼을 누르면 장바구니에 상품이 추가됨과 동시에 수량 선택 버튼이 노출된다.Changes
Layout
Screenshot
KakaoTalk_Video_2026-05-11-16-40-39.mp4
To Reviewer
안녕하세요! 베르 😸 별터입니다! 이번 미션에서는 개인 일정이 계속 있었어서ㅠ 핑퐁을 많이 못 한 것이 아쉽습니다.. 😢
지난 1,2단계 머지해주시면서 남겨주셨던 코멘트 반영하였습니다!
바쁘실텐데 리뷰 남겨주셔서 감사합니다 !! 😄