[3단계 - 상태 관리 | 4단계 - HTTP Client] 바드 미션 제출합니다 #171
Open
byunghyunkim0 wants to merge 42 commits into
Open
Conversation
- 3단계, 4단계 기능 목록 작성 - 3단계, 4단계 단위 테스트 목록 작성 - 지난 과제 요약
- Money 도메인 로직 구현
- Quantity 도메인 로직 구현
- CartItem 도메인 로직 구현
- 관련 test 코드 수정
- Cart 도메인 로직 구현 - CartItem, Quantity 비교 로직 구현
- 상품이 0이면 + 버튼 - 상품이 0이 아니면 수량 조절 버튼
- UiModel 수정
- 미사용 로직 삭제
- 미사용 로직 삭제
- CartRoute 구현
- MockServer 구현 - ProductResponse 구현 - gradle 추가
- mockData 형식 변경
- cartDao 구현 - cart Entity 구현
- RecentProductDao 구현 - RecentProduct Entity 구현
- ProductListViewModelTest 수정
Walkthrough이 PR은 상태 홀더 아키텍처를 ViewModel/Flow 기반 설계로 전면 전환하고, Room 영속성과 MockWebServer 원격 데이터 소스를 통합했다. 도메인 계약(저장소 인터페이스) → 데이터 계층(Room, 원격) → 저장소 구현 → ViewModel → UI 라우트/컴포넌트 순서로 계층화되었다. 수량 변경 컴포넌트, 최근 상품 목록, 카운트 배지 등의 UI 개선이 포함되고, CartStateHolder/ProductListStateHolder/ProductDetailStateHolder는 완전히 제거되었다. 빌드 설정은 KSP, Room 컴파일러, OkHttp, MockWebServer 등 필수 의존성을 추가했다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
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/productdetail/ProductDetailScreen.kt (1)
65-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win하단 CTA가 본문을 가릴 수 있습니다.
Line 111-113의
CartPutButton을Box에 겹쳐 올리고 있는데, Line 65-109의 본문에는 그 높이만큼의 하단 여백이나 스크롤 처리가 없습니다. 이번 변경으로 최신 상품 섹션까지 추가돼서 작은 화면이나 큰 글꼴에서는 마지막 콘텐츠가 버튼 뒤로 들어갈 가능성이 있습니다.🤖 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 65 - 114, The CTA CartPutButton is overlaying the content because the Column body (containing PreviewableAsyncImage, Text, QuantityCounter and LatestProductItem) has no bottom inset; add space or system-aware padding equal to the button height so the last content can't be obscured—e.g., give the scrolling/column content a bottom padding or spacer matching the CartPutButton height or move CartPutButton into a Scaffold bottomBar so Compose applies proper insets; update references in ProductDetailScreen to adjust the Column/scroll container padding (or replace with Scaffold bottomBar) and ensure LatestProductItem, QuantityCounter and other bottom elements respect that inset.
🧹 Nitpick comments (15)
build.gradle.kts (1)
7-7: ⚡ Quick win버전 카탈로그 사용을 고려해보세요
KSP 플러그인의 버전이 하드코딩되어 있는데, 다른 플러그인들(3-6번 라인)은
alias(libs.plugins.*)를 사용하고 있습니다.일관성과 중앙화된 버전 관리를 위해:
gradle/libs.versions.toml파일에서 다른 플러그인들이 어떻게 정의되어 있는지 확인해보세요- KSP 플러그인도 동일한 방식으로 정의하면 어떤 이점이 있을까요?
🤖 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 `@build.gradle.kts` at line 7, Replace the hardcoded KSP plugin version with a version-catalog alias: add an entry in gradle/libs.versions.toml under [plugins] (e.g., a key like ksp = { id = "com.google.devtools.ksp", version = "2.3.4" }) and then change the build script's plugin line from id("com.google.devtools.ksp") version "2.3.4" apply false to use alias(libs.plugins.ksp) apply false so the KSP plugin is managed consistently with the other alias(libs.plugins.*) entries.app/src/main/java/woowacourse/shopping/ui/common/AddCartButton.kt (1)
21-31: ⚡ Quick win접근성과 테마 호환성을 개선할 수 있습니다
현재 구현에서 몇 가지 개선점을 고려해보세요:
고정 크기 (25번 라인):
size(40.dp)는 사용자의 글꼴 크기 설정을 무시합니다
- 접근성을 위해 어떤 방법으로 크기를 동적으로 조정할 수 있을까요?
하드코딩된 색상 (24번 라인):
Color.White는 다크 테마에서 문제가 될 수 있습니다
- Material 테마의 색상 시스템(
MaterialTheme.colorScheme)을 활용하면 어떨까요?아이콘 대비 (27-30번 라인): 아이콘에 tint가 없어 흰색 배경에서 보이지 않을 수 있습니다
- 적절한 대비를 확보하려면 어떤 색상을 사용해야 할까요?
Compose Material Design 가이드라인의 "Theming"과 "Accessibility" 섹션을 참고해보세요.
🤖 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/common/AddCartButton.kt` around lines 21 - 31, Replace the hardcoded sizing and colors in the Surface/Icon block to respect user font scaling and Material theming: avoid modifier.size(40.dp) and instead derive size from the system font scale (e.g., multiply the base dp by LocalDensity.current.fontScale) or use standard Compose components like IconButton/Icons with IconButtonDefaults sizes; replace Color.White with a MaterialTheme color (e.g., MaterialTheme.colorScheme.surface or background color variable) and set the Icon tint to a contrasting on-color (e.g., MaterialTheme.colorScheme.onSurface or contentColorFor(backgroundColor)) so the icon remains visible in dark/light themes; update the Surface (and Icon) usages accordingly (Surface, modifier.size(...), Color.White, Icon(...), painterResource) to use the theme-derived colors and scaled size.app/src/main/res/drawable/add_icon.xml (1)
2-5: 💤 Low value아이콘 크기 정의를 검토해보세요
intrinsic 크기가 800dp × 800dp로 설정되어 있는데, viewport는 25×25입니다.
다음을 고려해보세요:
- 일반적으로 아이콘의 intrinsic width/height는 어떤 값을 사용하나요? (예: 24dp, 48dp 등)
- 현재 설정이 아이콘 렌더링이나 메모리 사용에 영향을 줄 수 있을까요?
- viewport와 intrinsic dimensions를 일치시키거나 표준 아이콘 크기(24dp)로 변경하면 어떨까요?
🤖 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/res/drawable/add_icon.xml` around lines 2 - 5, The icon's intrinsic size is incorrectly set to 800dp while the viewport is 25×25; update the android:width and android:height attributes in add_icon.xml to a standard icon size such as "24dp" (or make them match the viewport, e.g., "25dp" if your vector coordinates expect 25 units) so the intrinsic dimensions and viewport are consistent and avoid excessive memory/rendering overhead; adjust android:width/android:height (and if necessary android:viewportWidth/android:viewportHeight) in the same file to keep coordinate scaling correct.app/src/main/java/woowacourse/shopping/ui/model/DetailProductUiModel.kt (1)
20-20: 💤 Low valueDecimalFormat의 로케일 설정을 고려해보셨나요?
현재
DecimalFormat("#,###")을 사용하고 있는데, 명시적인 로케일 없이 시스템 기본값을 사용하고 있습니다.다음 질문들을 생각해보시면 도움이 될 것 같습니다:
- 다른 로케일 환경에서 앱이 실행될 때 어떤 일이 발생할까요?
- 숫자 형식(천 단위 구분자 등)이 항상 한국 형식으로 표시되도록 보장할 방법은 무엇일까요?
힌트:
DecimalFormat의 생성자나 관련 메서드에서Locale을 지정할 수 있습니다.🤖 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/model/DetailProductUiModel.kt` at line 20, The current formattedPrice uses DecimalFormat("#,###") without a locale; change it to use a locale-aware formatter to guarantee Korean formatting (e.g., use NumberFormat.getIntegerInstance(Locale.KOREA) or new DecimalFormat(pattern, DecimalFormatSymbols(Locale.KOREA)) ) when computing formattedPrice in DetailProductUiModel so the thousands separator is always Korean-style before appending "원".app/src/main/java/woowacourse/shopping/data/remote/api/ShoppingMockServer.kt (1)
43-57: 🏗️ Heavy liftMockWebServer의
start()동작과 테스트 타이밍을 생각해보세요.현재 코드에서
server.start(PORT)가 백그라운드 스레드에서 비동기로 실행되고 있습니다(Line 50-56). MockWebServer의start()메서드가 실제로 어떻게 동작하는지 생각해봅시다:
- MockWebServer의
start()메서드가 동기적으로 포트 바인딩을 완료한 후 반환하는지, 아니면 비동기로 시작되는지 확인해보세요.- 현재 테스트 패턴에서
setUp()이 끝나고 실제@Test메서드가 실행되기까지 어느 정도의 시간 차이가 있는지 고려해봤나요?테스트를 여러 번 실행하면서 간헐적인 연결 실패(
ConnectException, 타임아웃 등)가 발생하는지 확인한 후, MockWebServer 문서를 참고하여 더 안전한 동기화 메커니즘(예:CountDownLatch)을 도입할 필요가 있는지 판단하는 것을 권장합니다.🤖 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/data/remote/api/ShoppingMockServer.kt` around lines 43 - 57, The start() currently launches MockWebServer.start(PORT) asynchronously on a new Thread which can cause races in tests; change start() (the function in ShoppingMockServer) to start the MockWebServer synchronously or block until it is ready by replacing the fire-and-forget Thread/try-catch with a startup sequence that calls server.start(PORT) on the current thread or uses a synchronization primitive (e.g., CountDownLatch) to wait for server to bind before returning; ensure you still assign mockWebServer and server.dispatcher before waiting and surface any exceptions from server.start(PORT) instead of just printing the stack trace so tests fail fast.app/build.gradle.kts (1)
87-87: ⚡ Quick win프로덕션 의존성에 MockWebServer가 포함되어 있습니다.
MockWebServer는 일반적으로 테스트용 라이브러리로,testImplementation또는androidTestImplementation스코프에 선언됩니다.
MainActivity.kt에서ShoppingMockServer를 실제로 사용하고 있어 의도적인 것으로 보이지만, 프로덕션과 개발/테스트 환경을 분리하는 방법을 고려해보시면 어떨까요?힌트:
- BuildConfig 또는 Gradle build variants를 활용하여 환경을 구분할 수 있습니다
- 실제 API와 Mock API를 인터페이스로 추상화하고, 환경에 따라 구현체를 주입하는 방식도 가능합니다
이러한 패턴에 대해 더 알아보고 싶으시다면, Android의 Dependency Injection 및 Build Variants 문서를 참고해보세요.
As per coding guidelines: Guide the programmer in problem-solving instead of providing direct answers and refer to relevant documentation instead of providing ready-made solutions.
🤖 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` at line 87, The build currently declares implementation(libs.mockwebserver3) which pulls MockWebServer into production; change this by moving the MockWebServer dependency out of the production implementation scope (use testImplementation or androidTestImplementation) or make its inclusion conditional via build variants/BuildConfig so production builds don't include it; update MainActivity.kt usage of ShoppingMockServer by abstracting the server behind an interface or using DI/BuildConfig flags to inject either the real API implementation or the ShoppingMockServer mock based on the build variant; refer to Android Build Variants and DI docs when implementing the conditional wiring.app/src/test/java/woowacourse/shopping/data/remote/source/ProductRemoteDataSourceTest.kt (1)
3-3: ⚡ Quick win코루틴 테스트를 위한 더 나은 방법을 고려해보세요.
현재
runBlocking을 사용하고 있는데, 프로젝트에 이미 포함된kotlinx-coroutines-test라이브러리의runTest를 사용하면 어떤 이점이 있을지 생각해보세요.질문:
runBlocking과runTest의 차이점은 무엇일까요?- 테스트 실행 시간이나 가상 시간 제어 측면에서 어떤 차이가 있을까요?
코루틴 테스트 모범 사례에 대해서는 Kotlin 공식 문서의 "Testing Kotlin coroutines" 섹션을 참고해보시기 바랍니다.
As per coding guidelines: Ask leading questions and provide hints instead of just telling the answer, and refer to relevant documentation instead of providing ready-made solutions.
Also applies to: 25-25, 33-33
🤖 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/data/remote/source/ProductRemoteDataSourceTest.kt` at line 3, 현재 ProductRemoteDataSourceTest.kt에서 kotlinx.coroutines.runBlocking을 사용하고 있는데, kotlinx-coroutines-test의 runTest로 교체하세요: import문을 kotlinx.coroutines.test.runTest로 바꾸고 테스트 케이스들(즉 테스트 함수들에서 runBlocking을 감싼 부분)을 runTest { ... }로 변경해 비동기 스케줄과 가상 시간 제어를 활용할 수 있게 하며, 필요하면 TestScope/advanceTimeBy 같은 가상 시간 API를 사용해 시간 기반 검증을 구현하도록 하세요; 또한 빌드 의존성에 kotlinx-coroutines-test가 포함되어 있는지 확인하세요.app/schemas/woowacourse.shopping.data.local.AppDatabase/1.json (1)
77-82: ⚡ Quick win
recent_products테이블의viewedAt컬럼에 인덱스를 추가하는 것을 검토해보세요.
RecentProductDao의getRecentProducts()쿼리에서viewedAt으로 정렬하고 있으며, 현재 스키마에는 이 컬럼에 대한 인덱스가 없습니다.생각해볼 점:
- ORDER BY 절이 인덱싱된 컬럼을 사용하면 쿼리 성능이 어떻게 개선될까요?
- 테이블 데이터가 증가할수록 인덱스의 효과는 어떻게 변할까요?
- 인덱스 추가 시 INSERT/UPDATE 성능과 저장 공간의 트레이드오프를 고려했나요?
Room에서 인덱스를 정의하는 방법은
@Index어노테이션을 참고하시고, SQLite의 쿼리 최적화 가이드를 살펴보시면 더 깊은 이해에 도움이 될 것입니다.🤖 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/schemas/woowacourse.shopping.data.local.AppDatabase/1.json` around lines 77 - 82, The recent_products table lacks an index on the viewedAt column while RecentProductDao.getRecentProducts() sorts by viewedAt; add an index for viewedAt (e.g., via Room's `@Index` on the entity or in the database schema for the recent_products table) to speed ORDER BY queries, and weigh the trade-offs (insertion/update overhead and storage). Locate the RecentProduct entity definition (entity class or schema for recent_products) and add an `@Index`(value = "viewedAt") (or equivalent JSON schema index entry for viewedAt) so the DAO's getRecentProducts() can leverage it. Ensure migrations are added/updated to create the new index safely.app/src/test/java/woowacourse/shopping/domain/CartItemTest.kt (1)
63-77: ⚡ Quick win감소 연산 경계값 테스트를 한 케이스 더 고정해 주세요.
현재는 정상 감소 경로만 검증합니다.
decrease에 현재 수량과 같거나 더 큰 값을 넣었을 때 정책(예외/삭제/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/test/java/woowacourse/shopping/domain/CartItemTest.kt` around lines 63 - 77, Add a boundary unit test in CartItemTest.kt that covers CartItem.decrease when the decrement is equal to or larger than the current Quantity: create a CartItem with a known Quantity (e.g., Quantity(5)) and call decrease with Quantity(5) and Quantity(6) and assert the domain's expected policy (e.g., assertThrows for IllegalArgumentException or the actual exception type used by CartItem.decrease) so the contract for decrease on non-positive-result cases is explicitly pinned; reference CartItem.decrease and Quantity in the new test.app/src/main/java/woowacourse/shopping/ui/productdetail/viewmodel/ProductDetailViewModel.kt (3)
35-35: ⚡ Quick winproduct_id가 누락된 경우의 처리를 검토해보세요.
현재
savedStateHandle["product_id"]가 null일 때 빈 문자열("")로 대체하고 있습니다. 이 경우loadProduct()에서 상품을 찾지 못해isError = true가 설정되지만, 근본 원인(잘못된 네비게이션 또는 누락된 인자)과 실제 상품 조회 실패를 구분하기 어렵습니다.다음을 고려해보세요:
- product_id가 필수 인자인 화면에서 이 값이 누락되었을 때 어떻게 처리하는 것이 적절할까요?
- 빈 문자열 대신 다른 처리 방법은 없을까요?
- 에러 상태에 실패 원인을 구분할 수 있는 정보를 추가하면 어떨까요?
🤖 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/viewmodel/ProductDetailViewModel.kt` at line 35, The current initialization of productId using savedStateHandle["product_id"] ?: "" masks a missing required navigation argument; update ProductDetailViewModel to explicitly detect a null product_id (e.g., check savedStateHandle.get<String>("product_id") == null) and handle it separately from loadProduct failures: set a distinct invalid-argument state (e.g., an invalidProductId or errorReason field) or throw a clear IllegalStateException/emit a one-off navigation event so the UI can show a specific "missing product id" message or navigate back, and avoid using an empty string as a sentinel; ensure references to productId, loadProduct(), and isError are updated to treat missing-id as a different error path.
76-83: ⚖️ Poor tradeoff장바구니 추가 실패 시 사용자 피드백을 고려해보세요.
cartRepository.addCartItem()이 실패할 경우(예: 네트워크 오류, 데이터베이스 오류) 현재는 아무런 피드백이 없습니다.다음을 생각해보세요:
- 사용자가 장바구니에 담기 버튼을 눌렀는데 실패했을 때, 어떤 피드백을 제공하는 것이 좋을까요?
UiState에 성공/실패 메시지나 스낵바 이벤트를 추가하는 것은 어떨까요?- try-catch로 예외를 잡아 처리하는 방법도 있습니다.
🤖 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/viewmodel/ProductDetailViewModel.kt` around lines 76 - 83, The addCartItem method should handle failures and surface user feedback: inside viewModelScope.launch in ProductDetailViewModel.addCartItem, wrap cartRepository.addCartItem(product, quantity) in try-catch and on success/failure update the ViewModel's UI state (e.g., add or set a snackbar/event field such as snackbarMessage or cartOperationResult on the UiState) so the UI can show a success message or an error message; ensure you reference currentProduct, _uiState, selectedQuantity, and cartRepository.addCartItem when implementing the try-catch and state update.
128-148: ⚖️ Poor tradeoffFactory에서 의존성 인스턴스 생성 방식을 검토해보세요.
Factory가 호출될 때마다
ProductRemoteDataSource(),ProductRepositoryImpl()등의 새 인스턴스를 생성합니다.다음을 고려해보세요:
ProductRemoteDataSource가 HTTP 클라이언트나 무거운 리소스를 포함한다면, ViewModel이 재생성될 때마다 새로 생성하는 것이 적절할까요?- Application 레벨에서 싱글톤으로 관리하거나 DI 프레임워크(Hilt, Koin 등)를 사용하는 것을 고려해보셨나요?
- 현재 구조에서 여러 ViewModel이 같은 데이터를 중복 로드하는 문제는 없을까요?
🤖 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/viewmodel/ProductDetailViewModel.kt` around lines 128 - 148, The Factory currently instantiates heavyweight dependencies per ViewModel creation (ProductRemoteDataSource, ProductRepositoryImpl, CartRepositoryImpl, RecentProductRepositoryImpl), which can cause redundant resource allocation; refactor so Factory obtains shared instances instead of new ones each call—e.g., retrieve singletons from AppDatabase.getDatabase(context) for DAOs and expose application-scoped instances (or integrate DI) for ProductRemoteDataSource and ProductRepositoryImpl, then pass those shared instances into ProductDetailViewModel(savedStateHandle = createSavedStateHandle(), productRepository = /* shared */, cartRepository = /* shared */, recentProductRepository = /* shared */) to avoid repeated construction and duplicated data loads.app/src/main/java/woowacourse/shopping/domain/Cart.kt (1)
40-56: ⚖️ Poor tradeoff도메인 모델에서 예외를 던지는 방식에 대해 고민해보세요.
minusProduct()가IllegalArgumentException을 던지는 방식으로 에러를 처리하고 있습니다. 이는 호출하는 쪽에서 반드시 예외 처리를 해야 함을 의미합니다.다음을 생각해보세요:
- Repository 계층에서 이 예외를 어떻게 처리하고 있나요? ViewModel까지 전파되나요?
- 도메인 로직의 검증 실패를 예외로 표현하는 것과
Result<Cart, CartError>같은 타입으로 표현하는 것의 장단점은 무엇일까요?- 예외 메시지가 한국어인데, 이것이 로깅/디버깅 시 문제가 될 수 있을까요?
Kotlin에서는
Result타입이나 sealed class를 사용한 에러 모델링도 고려해볼 수 있습니다.🤖 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/domain/Cart.kt` around lines 40 - 56, The domain method minusProduct in Cart currently throws IllegalArgumentException with Korean messages on validation failures; change it to return a typed error result instead (e.g., Result<Cart, CartError> or a sealed CartResult) so callers can handle failures without exceptions: introduce a sealed class or error enum (CartError) for cases like NotFound and InsufficientQuantity, update minusProduct to return the success case with Cart(...) or the appropriate CartError for findItem missing, quantity too large, or exact-match removal (use removeCartItem result wrapped), and update any callers to handle the new return type; keep references to minusProduct, Cart, cartItems, findItem, removeCartItem and the existing checks (isQuantityLessThan, isSameQuantity) when implementing this change.app/src/test/java/woowacourse/shopping/ui/productlist/ProductListViewModelTest.kt (1)
104-129: ⚡ Quick winMock 저장소의 모든 메서드가 테스트되고 있는지 확인해보세요.
현재 테스트는
addCartItem과decreaseCartItem을 검증하지만,MockCartRepository의deleteCartItem은 사용되지 않습니다.다음을 고려해보세요:
- 전체 삭제 기능(수량 감소가 아닌 항목 제거)을 테스트하는 케이스가 필요할까요?
- Mock 구현에 실제로 사용되지 않는 메서드가 있다면, 제거하거나 테스트를 추가하는 것이 좋을까요?
🤖 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/ui/productlist/ProductListViewModelTest.kt` around lines 104 - 129, Tests exercise addCartItem and decreaseCartItem paths but never invoke MockCartRepository.deleteCartItem, so add a new test that triggers the repository's delete path—either by calling the ViewModel method that maps to a full-delete (e.g., ProductListViewModel.removeCartItem with quantity==1 to cause deletion) or directly invoking cartRepository.deleteCartItem to simulate full removal; ensure the test sets up an item with quantity 1, calls the appropriate ViewModel or repository method, advances coroutines (advanceUntilIdle), then asserts the item is no longer present (cartRepository.getCart().first() is empty) to cover MockCartRepository.deleteCartItem.app/src/main/java/woowacourse/shopping/ui/productlist/viewmodel/ProductListViewModel.kt (1)
47-64: ⚖️ Poor tradeoff상태 업데이트가 두 곳에서 발생하는 구조를 검토해보세요.
observeDatabase()는 cart/recentProducts 변경 시uiState.products를 업데이트하고,syncUiState()는_products변경 시 동일한 작업을 수행합니다.다음을 고려해보세요:
_products가MutableList가 아닌StateFlow였다면 모든 상태 업데이트를 한 곳에서 처리할 수 있지 않을까요?- 현재 방식에서
observeDatabase와syncUiState의 매핑 로직(56-58라인, 107-109라인)이 중복되는데, 이를 하나로 통합할 방법은 없을까요?combine블록에_productsFlow를 추가하면 어떻게 될까요?현재 코드는 동작하지만, 상태 관리 로직이 분산되어 있어 유지보수 시 주의가 필요합니다.
🤖 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/productlist/viewmodel/ProductListViewModel.kt` around lines 47 - 64, observeDatabase() and syncUiState() duplicate the same mapping logic for building uiState.products and other fields; make _products a StateFlow (instead of a MutableList) and include it in the combine alongside cartRepository.getCart() and recentProductRepository.getRecentProducts(), then move the mapping that calls toDetailProductUiModel/toSimpleProductUiModel and sets cartCount into that single combined block (replace the existing mapping in observeDatabase() and remove the duplicate in syncUiState()); update usages of _products to emit/update the StateFlow and keep launching the combined flow in viewModelScope so all state updates are centralized.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@app/src/main/AndroidManifest.xml`:
- Line 8: The manifest currently sets usesCleartextTraffic="true" globally which
is unsafe; remove or set usesCleartextTraffic="false" and instead create a
Network Security Configuration XML (referenced by android:networkSecurityConfig)
that explicitly allows cleartext only for localhost, then scope that config to
debug builds only by either providing a debug-only AndroidManifest override that
adds android:networkSecurityConfig="@xml/network_security_config" or by using
buildType manifest placeholders (debug buildType injects the
networkSecurityConfig reference while release does not); refer to the symbols
usesCleartextTraffic, networkSecurityConfig, localhost and the debug buildType
when making the changes.
In `@app/src/main/java/woowacourse/shopping/data/local/dao/CartDao.kt`:
- Around line 12-13: The SELECT in CartDao.getCartItems() returns rows with no
defined order; update the query to include an explicit ORDER BY on the intended
sort column (e.g., ORDER BY id ASC for insertion order or ORDER BY created_at
DESC for newest-first) so cart_items are returned deterministically; modify the
`@Query`("SELECT * FROM cart_items") in CartDao to include that ORDER BY clause
(and adjust tests if they depend on a specific ordering).
In `@app/src/main/java/woowacourse/shopping/data/NetworkConfig.kt`:
- Around line 9-11: The logging interceptor in NetworkConfig.kt is set to
HttpLoggingInterceptor.Level.BODY which exposes request/response bodies; change
it to pick the level conditionally (use BuildConfig.DEBUG) so in debug builds
use BODY but in production use a safer level like NONE or BASIC. Locate the
loggingInterceptor creation (variable named loggingInterceptor, class
NetworkConfig) and modify the level assignment to check BuildConfig.DEBUG and
set Level.BODY for debug, otherwise Level.NONE (or Level.BASIC) to avoid leaking
sensitive data.
In
`@app/src/main/java/woowacourse/shopping/data/remote/api/ShoppingMockServer.kt`:
- Around line 29-37: In ShoppingMockServer's path.startsWith("/products/")
branch the code assigns response = MockData.getProductResponse(id) ?:
MockResponse.Builder().code(404).build() but then always proceeds to build a 200
response with gson.toJson(response); when getProductResponse(id) is null you end
up serializing a MockResponse object and returning 200 instead of the intended
404. Fix this by checking the result of MockData.getProductResponse(id)
explicitly: if it is null return/construct the 404 MockResponse immediately,
otherwise serialize the product response with gson.toJson(response) and return a
200; adjust the branch around getProductResponse, response, and the
MockResponse.Builder usage accordingly.
In
`@app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt`:
- Around line 20-48: The addCartItem and decreaseCartItem implementations read
current state via cartDao.getCartItems(), compute a new Cart in memory (using
Cart.plusProduct and Cart.minusProduct) and then write back with
cartDao.insertOrUpdate/deleteById, which is not atomic and can lose updates
under concurrent calls; fix by making the update atomic—either wrap the
read-modify-write in a database transaction provided by your DAO/DB layer or add
atomic DAO operations (e.g., increment/decrement quantity by product id or
upsert-with-compare) and use those instead of getCartItems()+insertOrUpdate;
update addCartItem, decreaseCartItem to call the transactional/atomic DAO
methods (or use the DAO’s transaction block) so concurrent coroutines cannot
overwrite each other.
In
`@app/src/main/java/woowacourse/shopping/data/repository/ProductRepositoryImpl.kt`:
- Around line 14-18: The catch currently swallows all Exceptions in
ProductRepositoryImpl.getProduct (which wraps
remoteDataSource.getProduct(id).toProduct()); change it to handle errors
explicitly: rethrow kotlinx.coroutines.CancellationException so cancellation
propagates, catch and return null only for a 404/NotFound-type error from the
remote layer (or map the specific HTTP/ApiException that indicates "not found"),
and let other transient errors (network/parsing) either be logged with full
details via the logger or propagated so they can be retried/handled upstream;
ensure you reference getProduct and remoteDataSource.getProduct when making
these changes and include detailed error info in logs instead of the current
println.
In `@app/src/main/java/woowacourse/shopping/domain/Money.kt`:
- Around line 9-13: plus와 times 연산에서 Int 오버플로우가 발생하면 wrap되어 잘못된 Money가 만들어질 수
있으므로 이를 방어하세요: Money.plus(otherMoney)와 Money.times(quantity)에서 연산 전에
Math.addExact / Math.multiplyExact를 사용하거나 연산을 long으로 수행해 결과가 Int 범위(>=0 && <=
Int.MAX_VALUE)에 있는지 검사한 뒤 유효하지 않으면 IllegalArgumentException을 던지도록 변경하고, 기존 생성자의
require(amount >= 0) 검증과 일관되게 음수/오버플로우 결과는 모두 예외 처리되게 하세요.
In `@app/src/main/java/woowacourse/shopping/domain/Quantity.kt`:
- Line 6: The validation in Quantity (the require(...) call referencing count)
and its error message are inconsistent: either make the rule exclude zero by
changing the predicate to count > 0 (so "수량은 1보다 작을 수 없습니다. 수량 : $count" remains
correct) or keep count >= 0 and update the message to match (e.g. "수량은 0보다 작을 수
없습니다. 수량 : $count"); update the require(...) in Quantity.kt accordingly so the
predicate and message refer to the same threshold.
In `@app/src/main/java/woowacourse/shopping/MainActivity.kt`:
- Line 17: The ShoppingMockServer lifecycle is vulnerable to race conditions
when Activity recreation calls ShoppingMockServer.start() while a background
thread is starting the server and onDestroy() calls shutdown(); make
start()/shutdown() thread-safe and idempotent or move initialization to
Application-level to guarantee single initialization. Update
ShoppingMockServer.start() to synchronize access (e.g., use a lock or atomic
compare-and-set on mockWebServer or a started flag), ensure start() blocks or
signals only after the server is fully started, and make shutdown() acquire the
same lock and safely stop/clear the server and thread (wait or interrupt as
needed) so rapid Activity recreate won't leave a partially started server or
cause concurrent starts/stops; alternatively initialize ShoppingMockServer once
from Application.onCreate() and remove start()/shutdown() calls from
MainActivity to centralize lifecycle.
In `@app/src/main/java/woowacourse/shopping/ui/common/QuantityCounter.kt`:
- Around line 37-71: In QuantityCounter.kt update the decrement and increment
Box modifiers (the Boxes that call onDecrement and onIncrement) to add
accessibility semantics and ensure a 48dp minimum touch target: add
Modifier.semantics { role = Role.Button; contentDescription = "Decrease
quantity" } (and "Increase quantity" for the other Box) and enforce size with
Modifier.defaultMinSize(minWidth = 48.dp, minHeight = 48.dp) (or sizeIn if you
prefer); keep the central quantity Text unchanged. This makes the interactive
Boxes announce as buttons with clear descriptions and meets the Android minimum
touch-target guideline.
In
`@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.kt`:
- Around line 17-35: ProductDetailActivity currently reuses the Activity with
FLAG_ACTIVITY_CLEAR_TOP so the ViewModel's productId (used in
ProductDetailRoute) isn’t updated; implement onNewIntent(intent: Intent) in
ProductDetailActivity to read EXTRA_PRODUCT_ID from the new intent, validate it
(null/blank handling like in onCreate), and update the ViewModel’s productId
(e.g., call the existing ViewModel method that sets/loads productId) or
re-trigger product load logic so the UI reflects the new product; alternatively,
if you prefer recreation, remove/change the flag where newIntent(...) is created
to force a fresh Activity instance.
In
`@app/src/main/java/woowacourse/shopping/ui/productdetail/viewmodel/ProductDetailViewModel.kt`:
- Around line 85-107: The increment() and decrement() functions currently
force-unwrap currentProduct (currentProduct!!) which can crash if loadProduct()
hasn't set it; change both to guard against null by returning early when
currentProduct is null (e.g., check currentProduct at start of each function or
inside the _uiState.update lambda and return the unchanged state), and only call
toDetailProductUiModel(currentProduct, Quantity(...)) when currentProduct is
non-null; alternatively ensure the UI disables quantity buttons until uiState
indicates product is loaded, but implement the null-check guard in
increment()/decrement() (referencing functions increment, decrement and the
currentProduct variable) to prevent the crash.
In `@app/src/main/java/woowacourse/shopping/ui/productlist/RecentProductList.kt`:
- Around line 66-71: The Text composable in RecentProductList.kt uses
TextOverflow.Ellipsis but lacks a maxLines constraint, which allows long titles
to wrap and break layout; update the Text call that renders title (the Text with
fontSize = 12.sp and fontWeight = W700) to include maxLines = 1 (same behavior
as SingleProductItem's title) so long product titles are truncated on a single
line with an ellipsis.
In
`@app/src/main/java/woowacourse/shopping/ui/productlist/viewmodel/ProductListViewModel.kt`:
- Around line 66-75: Replace the println exception handling in loadInitData()
with proper UI state updates: add/loading and error fields to ProductListUiState
(e.g., isLoading: Boolean, isError: Boolean, errorMessage: String?), set
isLoading=true before the repository call and false in finally, and on catch set
isError=true and errorMessage to e.message (or a user-friendly string); expose
the updated state (StateFlow/LiveData) to the UI and add a public retry method
(e.g., retryLoad() that calls loadInitData() or fetchProducts()) so the UI can
show an error view with a retry button—mirroring ProductDetailViewModel’s
isError approach and updating fetchProducts()/loadInitData() to emit these
states.
In
`@app/src/test/java/woowacourse/shopping/ui/productlist/ProductListViewModelTest.kt`:
- Around line 180-184: The deleteCartItem implementation currently keeps items
matching productId because cartItems.value = cartItems.value.filter {
it.hasProductId(productId) } uses filter to retain true matches; update the
predicate to exclude the target product by negating the check (use
!it.hasProductId(productId)) so cartItems.value contains all items except the
one to delete in the deleteCartItem suspend function, referencing
cartItems.value and the hasProductId method.
In `@README.md`:
- Around line 76-79: Fix the Korean particles in the README test descriptions:
change "Cart에 상품을 존재한다면" to "Cart에 상품이 존재한다면", "Cart에 상품을 존재하지 않는다면" to "Cart에
상품이 존재하지 않는다면", and update both occurrences of "Cart에 입력받은" to "Cart에서 입력받은" so
the phrases use the correct subject particle '이' for existence and the location
particle '에서' for receiving; update the four checklist lines accordingly.
- Around line 80-85: Remove the stray spaces before Korean particles in the
checklist lines so particles are attached to the preceding word (e.g. change
"Cart 를" to "Cart를", "Cart 를 반환한다" -> "Cart를 반환한다"); scan the README.md
checklist items for other occurrences of space-before-particle patterns (를, 을,
은, 는, 에, 의, 로, 와/과 등) and correct them so each particle is directly appended to
the prior token.
---
Outside diff comments:
In
`@app/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailScreen.kt`:
- Around line 65-114: The CTA CartPutButton is overlaying the content because
the Column body (containing PreviewableAsyncImage, Text, QuantityCounter and
LatestProductItem) has no bottom inset; add space or system-aware padding equal
to the button height so the last content can't be obscured—e.g., give the
scrolling/column content a bottom padding or spacer matching the CartPutButton
height or move CartPutButton into a Scaffold bottomBar so Compose applies proper
insets; update references in ProductDetailScreen to adjust the Column/scroll
container padding (or replace with Scaffold bottomBar) and ensure
LatestProductItem, QuantityCounter and other bottom elements respect that inset.
---
Nitpick comments:
In `@app/build.gradle.kts`:
- Line 87: The build currently declares implementation(libs.mockwebserver3)
which pulls MockWebServer into production; change this by moving the
MockWebServer dependency out of the production implementation scope (use
testImplementation or androidTestImplementation) or make its inclusion
conditional via build variants/BuildConfig so production builds don't include
it; update MainActivity.kt usage of ShoppingMockServer by abstracting the server
behind an interface or using DI/BuildConfig flags to inject either the real API
implementation or the ShoppingMockServer mock based on the build variant; refer
to Android Build Variants and DI docs when implementing the conditional wiring.
In `@app/schemas/woowacourse.shopping.data.local.AppDatabase/1.json`:
- Around line 77-82: The recent_products table lacks an index on the viewedAt
column while RecentProductDao.getRecentProducts() sorts by viewedAt; add an
index for viewedAt (e.g., via Room's `@Index` on the entity or in the database
schema for the recent_products table) to speed ORDER BY queries, and weigh the
trade-offs (insertion/update overhead and storage). Locate the RecentProduct
entity definition (entity class or schema for recent_products) and add an
`@Index`(value = "viewedAt") (or equivalent JSON schema index entry for viewedAt)
so the DAO's getRecentProducts() can leverage it. Ensure migrations are
added/updated to create the new index safely.
In
`@app/src/main/java/woowacourse/shopping/data/remote/api/ShoppingMockServer.kt`:
- Around line 43-57: The start() currently launches MockWebServer.start(PORT)
asynchronously on a new Thread which can cause races in tests; change start()
(the function in ShoppingMockServer) to start the MockWebServer synchronously or
block until it is ready by replacing the fire-and-forget Thread/try-catch with a
startup sequence that calls server.start(PORT) on the current thread or uses a
synchronization primitive (e.g., CountDownLatch) to wait for server to bind
before returning; ensure you still assign mockWebServer and server.dispatcher
before waiting and surface any exceptions from server.start(PORT) instead of
just printing the stack trace so tests fail fast.
In `@app/src/main/java/woowacourse/shopping/domain/Cart.kt`:
- Around line 40-56: The domain method minusProduct in Cart currently throws
IllegalArgumentException with Korean messages on validation failures; change it
to return a typed error result instead (e.g., Result<Cart, CartError> or a
sealed CartResult) so callers can handle failures without exceptions: introduce
a sealed class or error enum (CartError) for cases like NotFound and
InsufficientQuantity, update minusProduct to return the success case with
Cart(...) or the appropriate CartError for findItem missing, quantity too large,
or exact-match removal (use removeCartItem result wrapped), and update any
callers to handle the new return type; keep references to minusProduct, Cart,
cartItems, findItem, removeCartItem and the existing checks (isQuantityLessThan,
isSameQuantity) when implementing this change.
In `@app/src/main/java/woowacourse/shopping/ui/common/AddCartButton.kt`:
- Around line 21-31: Replace the hardcoded sizing and colors in the Surface/Icon
block to respect user font scaling and Material theming: avoid
modifier.size(40.dp) and instead derive size from the system font scale (e.g.,
multiply the base dp by LocalDensity.current.fontScale) or use standard Compose
components like IconButton/Icons with IconButtonDefaults sizes; replace
Color.White with a MaterialTheme color (e.g., MaterialTheme.colorScheme.surface
or background color variable) and set the Icon tint to a contrasting on-color
(e.g., MaterialTheme.colorScheme.onSurface or contentColorFor(backgroundColor))
so the icon remains visible in dark/light themes; update the Surface (and Icon)
usages accordingly (Surface, modifier.size(...), Color.White, Icon(...),
painterResource) to use the theme-derived colors and scaled size.
In `@app/src/main/java/woowacourse/shopping/ui/model/DetailProductUiModel.kt`:
- Line 20: The current formattedPrice uses DecimalFormat("#,###") without a
locale; change it to use a locale-aware formatter to guarantee Korean formatting
(e.g., use NumberFormat.getIntegerInstance(Locale.KOREA) or new
DecimalFormat(pattern, DecimalFormatSymbols(Locale.KOREA)) ) when computing
formattedPrice in DetailProductUiModel so the thousands separator is always
Korean-style before appending "원".
In
`@app/src/main/java/woowacourse/shopping/ui/productdetail/viewmodel/ProductDetailViewModel.kt`:
- Line 35: The current initialization of productId using
savedStateHandle["product_id"] ?: "" masks a missing required navigation
argument; update ProductDetailViewModel to explicitly detect a null product_id
(e.g., check savedStateHandle.get<String>("product_id") == null) and handle it
separately from loadProduct failures: set a distinct invalid-argument state
(e.g., an invalidProductId or errorReason field) or throw a clear
IllegalStateException/emit a one-off navigation event so the UI can show a
specific "missing product id" message or navigate back, and avoid using an empty
string as a sentinel; ensure references to productId, loadProduct(), and isError
are updated to treat missing-id as a different error path.
- Around line 76-83: The addCartItem method should handle failures and surface
user feedback: inside viewModelScope.launch in
ProductDetailViewModel.addCartItem, wrap cartRepository.addCartItem(product,
quantity) in try-catch and on success/failure update the ViewModel's UI state
(e.g., add or set a snackbar/event field such as snackbarMessage or
cartOperationResult on the UiState) so the UI can show a success message or an
error message; ensure you reference currentProduct, _uiState, selectedQuantity,
and cartRepository.addCartItem when implementing the try-catch and state update.
- Around line 128-148: The Factory currently instantiates heavyweight
dependencies per ViewModel creation (ProductRemoteDataSource,
ProductRepositoryImpl, CartRepositoryImpl, RecentProductRepositoryImpl), which
can cause redundant resource allocation; refactor so Factory obtains shared
instances instead of new ones each call—e.g., retrieve singletons from
AppDatabase.getDatabase(context) for DAOs and expose application-scoped
instances (or integrate DI) for ProductRemoteDataSource and
ProductRepositoryImpl, then pass those shared instances into
ProductDetailViewModel(savedStateHandle = createSavedStateHandle(),
productRepository = /* shared */, cartRepository = /* shared */,
recentProductRepository = /* shared */) to avoid repeated construction and
duplicated data loads.
In
`@app/src/main/java/woowacourse/shopping/ui/productlist/viewmodel/ProductListViewModel.kt`:
- Around line 47-64: observeDatabase() and syncUiState() duplicate the same
mapping logic for building uiState.products and other fields; make _products a
StateFlow (instead of a MutableList) and include it in the combine alongside
cartRepository.getCart() and recentProductRepository.getRecentProducts(), then
move the mapping that calls toDetailProductUiModel/toSimpleProductUiModel and
sets cartCount into that single combined block (replace the existing mapping in
observeDatabase() and remove the duplicate in syncUiState()); update usages of
_products to emit/update the StateFlow and keep launching the combined flow in
viewModelScope so all state updates are centralized.
In `@app/src/main/res/drawable/add_icon.xml`:
- Around line 2-5: The icon's intrinsic size is incorrectly set to 800dp while
the viewport is 25×25; update the android:width and android:height attributes in
add_icon.xml to a standard icon size such as "24dp" (or make them match the
viewport, e.g., "25dp" if your vector coordinates expect 25 units) so the
intrinsic dimensions and viewport are consistent and avoid excessive
memory/rendering overhead; adjust android:width/android:height (and if necessary
android:viewportWidth/android:viewportHeight) in the same file to keep
coordinate scaling correct.
In
`@app/src/test/java/woowacourse/shopping/data/remote/source/ProductRemoteDataSourceTest.kt`:
- Line 3: 현재 ProductRemoteDataSourceTest.kt에서 kotlinx.coroutines.runBlocking을
사용하고 있는데, kotlinx-coroutines-test의 runTest로 교체하세요: import문을
kotlinx.coroutines.test.runTest로 바꾸고 테스트 케이스들(즉 테스트 함수들에서 runBlocking을 감싼 부분)을
runTest { ... }로 변경해 비동기 스케줄과 가상 시간 제어를 활용할 수 있게 하며, 필요하면
TestScope/advanceTimeBy 같은 가상 시간 API를 사용해 시간 기반 검증을 구현하도록 하세요; 또한 빌드 의존성에
kotlinx-coroutines-test가 포함되어 있는지 확인하세요.
In `@app/src/test/java/woowacourse/shopping/domain/CartItemTest.kt`:
- Around line 63-77: Add a boundary unit test in CartItemTest.kt that covers
CartItem.decrease when the decrement is equal to or larger than the current
Quantity: create a CartItem with a known Quantity (e.g., Quantity(5)) and call
decrease with Quantity(5) and Quantity(6) and assert the domain's expected
policy (e.g., assertThrows for IllegalArgumentException or the actual exception
type used by CartItem.decrease) so the contract for decrease on
non-positive-result cases is explicitly pinned; reference CartItem.decrease and
Quantity in the new test.
In
`@app/src/test/java/woowacourse/shopping/ui/productlist/ProductListViewModelTest.kt`:
- Around line 104-129: Tests exercise addCartItem and decreaseCartItem paths but
never invoke MockCartRepository.deleteCartItem, so add a new test that triggers
the repository's delete path—either by calling the ViewModel method that maps to
a full-delete (e.g., ProductListViewModel.removeCartItem with quantity==1 to
cause deletion) or directly invoking cartRepository.deleteCartItem to simulate
full removal; ensure the test sets up an item with quantity 1, calls the
appropriate ViewModel or repository method, advances coroutines
(advanceUntilIdle), then asserts the item is no longer present
(cartRepository.getCart().first() is empty) to cover
MockCartRepository.deleteCartItem.
In `@build.gradle.kts`:
- Line 7: Replace the hardcoded KSP plugin version with a version-catalog alias:
add an entry in gradle/libs.versions.toml under [plugins] (e.g., a key like ksp
= { id = "com.google.devtools.ksp", version = "2.3.4" }) and then change the
build script's plugin line from id("com.google.devtools.ksp") version "2.3.4"
apply false to use alias(libs.plugins.ksp) apply false so the KSP plugin is
managed consistently with the other alias(libs.plugins.*) entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79498f24-093d-42b7-8228-c428ff261cbf
📒 Files selected for processing (76)
.editorconfigREADME.mdapp/build.gradle.ktsapp/schemas/woowacourse.shopping.data.local.AppDatabase/1.jsonapp/src/androidTest/java/woowacourse/shopping/data/local/dao/CartDaoTest.ktapp/src/androidTest/java/woowacourse/shopping/data/local/dao/RecentProductDaoTest.ktapp/src/main/AndroidManifest.xmlapp/src/main/java/woowacourse/shopping/MainActivity.ktapp/src/main/java/woowacourse/shopping/constants/MockData.ktapp/src/main/java/woowacourse/shopping/data/NetworkConfig.ktapp/src/main/java/woowacourse/shopping/data/local/AppDatabase.ktapp/src/main/java/woowacourse/shopping/data/local/dao/CartDao.ktapp/src/main/java/woowacourse/shopping/data/local/dao/RecentProductDao.ktapp/src/main/java/woowacourse/shopping/data/local/entity/CartItemEntity.ktapp/src/main/java/woowacourse/shopping/data/local/entity/RecentProductEntity.ktapp/src/main/java/woowacourse/shopping/data/remote/api/ShoppingMockServer.ktapp/src/main/java/woowacourse/shopping/data/remote/model/ProductResponse.ktapp/src/main/java/woowacourse/shopping/data/remote/source/ProductRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/ProductRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/RecentProductRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/domain/Cart.ktapp/src/main/java/woowacourse/shopping/domain/CartItem.ktapp/src/main/java/woowacourse/shopping/domain/Money.ktapp/src/main/java/woowacourse/shopping/domain/Product.ktapp/src/main/java/woowacourse/shopping/domain/Quantity.ktapp/src/main/java/woowacourse/shopping/domain/repository/CartRepository.ktapp/src/main/java/woowacourse/shopping/domain/repository/ProductRepository.ktapp/src/main/java/woowacourse/shopping/domain/repository/RecentProductRepository.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartActivity.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartAppBar.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartRoute.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartScreen.ktapp/src/main/java/woowacourse/shopping/ui/cart/SingleCartItem.ktapp/src/main/java/woowacourse/shopping/ui/cart/state/CartUiState.ktapp/src/main/java/woowacourse/shopping/ui/cart/stateholder/CartStateHolder.ktapp/src/main/java/woowacourse/shopping/ui/cart/viewmodel/CartViewModel.ktapp/src/main/java/woowacourse/shopping/ui/common/AddCartButton.ktapp/src/main/java/woowacourse/shopping/ui/common/QuantityCounter.ktapp/src/main/java/woowacourse/shopping/ui/model/DetailProductUiModel.ktapp/src/main/java/woowacourse/shopping/ui/model/LatestProductUiModel.ktapp/src/main/java/woowacourse/shopping/ui/model/SimpleProductUiModel.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductAppBar.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailActivity.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailRoute.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/ProductDetailScreen.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/state/ProductDetailUiState.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/stateholder/ProductDetailStateHolder.ktapp/src/main/java/woowacourse/shopping/ui/productdetail/viewmodel/ProductDetailViewModel.ktapp/src/main/java/woowacourse/shopping/ui/productlist/PreviewableAsyncImage.ktapp/src/main/java/woowacourse/shopping/ui/productlist/ProductList.ktapp/src/main/java/woowacourse/shopping/ui/productlist/ProductListAppBar.ktapp/src/main/java/woowacourse/shopping/ui/productlist/ProductListRoute.ktapp/src/main/java/woowacourse/shopping/ui/productlist/ProductListScreen.ktapp/src/main/java/woowacourse/shopping/ui/productlist/RecentProductList.ktapp/src/main/java/woowacourse/shopping/ui/productlist/SingleProductItem.ktapp/src/main/java/woowacourse/shopping/ui/productlist/state/ProductListUiState.ktapp/src/main/java/woowacourse/shopping/ui/productlist/stateholder/ProductListStateHolder.ktapp/src/main/java/woowacourse/shopping/ui/productlist/viewmodel/ProductListViewModel.ktapp/src/main/java/woowacourse/shopping/ui/state/ProductUiModel.ktapp/src/main/java/woowacourse/shopping/ui/theme/Theme.ktapp/src/main/res/drawable/add_icon.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/woowacourse/shopping/data/remote/source/ProductRemoteDataSourceTest.ktapp/src/test/java/woowacourse/shopping/data/repository/ProductRepositoryImplTest.ktapp/src/test/java/woowacourse/shopping/domain/CartItemTest.ktapp/src/test/java/woowacourse/shopping/domain/CartTest.ktapp/src/test/java/woowacourse/shopping/domain/MoneyTest.ktapp/src/test/java/woowacourse/shopping/domain/ProductTest.ktapp/src/test/java/woowacourse/shopping/domain/QuantityTest.ktapp/src/test/java/woowacourse/shopping/ui/cart/CartStateHolderTest.ktapp/src/test/java/woowacourse/shopping/ui/cart/CartViewModelTest.ktapp/src/test/java/woowacourse/shopping/ui/productlist/ProductListStateHolderTest.ktapp/src/test/java/woowacourse/shopping/ui/productlist/ProductListViewModelTest.ktbuild.gradle.ktsgradle/libs.versions.toml
💤 Files with no reviewable changes (6)
- app/src/main/java/woowacourse/shopping/ui/state/ProductUiModel.kt
- app/src/test/java/woowacourse/shopping/ui/cart/CartStateHolderTest.kt
- app/src/main/java/woowacourse/shopping/ui/productdetail/stateholder/ProductDetailStateHolder.kt
- app/src/test/java/woowacourse/shopping/ui/productlist/ProductListStateHolderTest.kt
- app/src/main/java/woowacourse/shopping/ui/cart/stateholder/CartStateHolder.kt
- app/src/main/java/woowacourse/shopping/ui/productlist/stateholder/ProductListStateHolder.kt
| <uses-permission android:name="android.permission.INTERNET" /> | ||
|
|
||
| <application | ||
| android:usesCleartextTraffic="true" |
There was a problem hiding this comment.
프로덕션 보안 위험: cleartext traffic 설정을 제한해야 합니다
usesCleartextTraffic="true"는 모든 HTTP 트래픽을 허용하므로 프로덕션 환경에서 보안 위험이 있습니다. 로컬 MockServer를 위해 필요하지만, 실제 배포 시에는 문제가 될 수 있습니다.
다음을 고려해보세요:
- Network Security Configuration을 사용하여 localhost에만 cleartext traffic을 허용하는 방법은 무엇일까요?
- Debug 빌드에서만 이 설정을 활성화하려면 어떤 방법이 있을까요?
Android 공식 문서의 "Network Security Configuration" 섹션을 참고하면 도움이 될 것입니다.
🤖 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/AndroidManifest.xml` at line 8, The manifest currently sets
usesCleartextTraffic="true" globally which is unsafe; remove or set
usesCleartextTraffic="false" and instead create a Network Security Configuration
XML (referenced by android:networkSecurityConfig) that explicitly allows
cleartext only for localhost, then scope that config to debug builds only by
either providing a debug-only AndroidManifest override that adds
android:networkSecurityConfig="@xml/network_security_config" or by using
buildType manifest placeholders (debug buildType injects the
networkSecurityConfig reference while release does not); refer to the symbols
usesCleartextTraffic, networkSecurityConfig, localhost and the debug buildType
when making the changes.
There was a problem hiding this comment.
이거는 참고해보시기 바랍니다. 지금 디버그 빌드를 했다면 이 옵션이 됐어야 했을텐데 왜 킨 것인지도 궁금하네요.
Comment on lines
+12
to
+13
| @Query("SELECT * FROM cart_items") | ||
| fun getCartItems(): Flow<List<CartItemEntity>> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine CartDao.kt file
find . -name "CartDao.kt" -type fRepository: woowacourse/android-shopping-cart
Length of output: 144
🏁 Script executed:
# Once found, let's look at the complete DAO file with context
cat -n "app/src/main/java/woowacourse/shopping/data/local/dao/CartDao.kt"Repository: woowacourse/android-shopping-cart
Length of output: 821
🏁 Script executed:
# Search for usages of getCartItems to understand how it's consumed
rg "getCartItems" --type kt -B 2 -A 2Repository: woowacourse/android-shopping-cart
Length of output: 104
🏁 Script executed:
# Check for any test files related to CartDao
find . -name "*CartDao*" -type f | grep -i testRepository: woowacourse/android-shopping-cart
Length of output: 155
🏁 Script executed:
# Look for the database schema to understand the cart_items table structure
rg "cart_items\|CartItemEntity" --type kt -B 2 -A 2 | head -50Repository: woowacourse/android-shopping-cart
Length of output: 104
🏁 Script executed:
# Examine the test file to see how getCartItems is tested
cat -n "app/src/androidTest/java/woowacourse/shopping/data/local/dao/CartDaoTest.kt"Repository: woowacourse/android-shopping-cart
Length of output: 2881
🏁 Script executed:
# Search for getCartItems usages without the type flag
rg "getCartItems" -B 3 -A 3Repository: woowacourse/android-shopping-cart
Length of output: 5762
🏁 Script executed:
# Look for CartItemEntity definition
rg "class CartItemEntity\|data class CartItemEntity" -B 2 -A 10Repository: woowacourse/android-shopping-cart
Length of output: 59
🏁 Script executed:
# Find where cart data is displayed/used in the UI
rg "getCartItems\|CartDao\|cart_items" --type-list | head -20Repository: woowacourse/android-shopping-cart
Length of output: 467
🏁 Script executed:
# Search for any ORDER BY in similar queries
rg "ORDER BY" -B 2 -A 2Repository: woowacourse/android-shopping-cart
Length of output: 619
🏁 Script executed:
# Check if there's any UI code that relies on cart order
rg "getCart\|CartRepositoryImpl\|cart_items" app/src/main/java -B 1 -A 3 | grep -A 5 "getCart"Repository: woowacourse/android-shopping-cart
Length of output: 59
🏁 Script executed:
# Look at CartItemEntity definition to see if there's any natural ordering field
rg "class CartItemEntity\|data class CartItemEntity" -B 2 -A 15Repository: woowacourse/android-shopping-cart
Length of output: 59
SELECT 쿼리의 반환 순서가 정의되지 않은 점을 고려해 보세요.
Line 12의 SELECT * FROM cart_items는 ORDER BY 절이 없어 데이터베이스가 결과를 어떤 순서로 반환할지 보장하지 않습니다. 현재 테스트는 단일 항목으로만 검증되고 있는데, 실제로 장바구니에 여러 상품이 있을 때는 어떻게 될까요?
다음 질문들을 생각해 보세요:
- 사용자가 장바구니 목록을 볼 때 특정 순서로 표시되기를 기대할까요? (예: 추가 순서, 상품명 순 등)
- 여러 항목이 반복되는 상황에서 순서가 일관되지 않으면 어떤 문제가 생길까요?
- 코드베이스의 다른 DAO들(예:
RecentProductDao)은 어떤 방식으로 이 문제를 해결하고 있는지 살펴보셨나요?
정렬 기준을 명시하면 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/data/local/dao/CartDao.kt` around
lines 12 - 13, The SELECT in CartDao.getCartItems() returns rows with no defined
order; update the query to include an explicit ORDER BY on the intended sort
column (e.g., ORDER BY id ASC for insertion order or ORDER BY created_at DESC
for newest-first) so cart_items are returned deterministically; modify the
`@Query`("SELECT * FROM cart_items") in CartDao to include that ORDER BY clause
(and adjust tests if they depend on a specific ordering).
Comment on lines
+9
to
+11
| val loggingInterceptor = HttpLoggingInterceptor().apply { | ||
| level = HttpLoggingInterceptor.Level.BODY | ||
| } |
There was a problem hiding this comment.
프로덕션에서 민감한 데이터 노출 위험이 있습니다
HttpLoggingInterceptor가 Level.BODY로 설정되어 있어 모든 요청/응답 본문이 로깅됩니다. 이는 프로덕션 환경에서 심각한 보안 및 개인정보 보호 문제가 될 수 있습니다.
다음을 생각해보세요:
- 프로덕션 빌드와 디버그 빌드를 구분하려면 어떤 방법을 사용할 수 있을까요?
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/data/NetworkConfig.kt` around lines 9
- 11, The logging interceptor in NetworkConfig.kt is set to
HttpLoggingInterceptor.Level.BODY which exposes request/response bodies; change
it to pick the level conditionally (use BuildConfig.DEBUG) so in debug builds
use BODY but in production use a safer level like NONE or BASIC. Locate the
loggingInterceptor creation (variable named loggingInterceptor, class
NetworkConfig) and modify the level assignment to check BuildConfig.DEBUG and
set Level.BODY for debug, otherwise Level.NONE (or Level.BASIC) to avoid leaking
sensitive data.
Comment on lines
+29
to
+37
| path.startsWith("/products/") -> { | ||
| val id = path.substringAfterLast("/") | ||
| val response = MockData.getProductResponse(id) ?: MockResponse.Builder().code(404).build() | ||
| MockResponse.Builder() | ||
| .code(200) | ||
| .body(gson.toJson(response)) | ||
| .addHeader("Content-Type", "application/json") | ||
| .build() | ||
| } |
There was a problem hiding this comment.
제품 조회 엔드포인트의 로직 오류를 수정해야 합니다.
Line 31에서 getProductResponse(id)가 null을 반환할 때 404 응답을 생성하지만, 이어지는 Lines 32-36이 항상 실행되어 새로운 200 응답으로 덮어씁니다.
getProductResponse가 null을 반환한 경우의 실행 흐름을 다시 살펴보시기 바랍니다. 현재 구조에서는 어떤 문제가 발생할까요?
힌트: Line 31의 response 변수가 MockResponse 타입일 때 Line 34의 gson.toJson(response)는 어떻게 동작할까요?
🤖 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/data/remote/api/ShoppingMockServer.kt`
around lines 29 - 37, In ShoppingMockServer's path.startsWith("/products/")
branch the code assigns response = MockData.getProductResponse(id) ?:
MockResponse.Builder().code(404).build() but then always proceeds to build a 200
response with gson.toJson(response); when getProductResponse(id) is null you end
up serializing a MockResponse object and returning 200 instead of the intended
404. Fix this by checking the result of MockData.getProductResponse(id)
explicitly: if it is null return/construct the 404 MockResponse immediately,
otherwise serialize the product response with gson.toJson(response) and return a
200; adjust the branch around getProductResponse, response, and the
MockResponse.Builder usage accordingly.
Comment on lines
+20
to
+48
| override suspend fun addCartItem( | ||
| product: Product, | ||
| quantity: Quantity, | ||
| ) { | ||
| val currentEntity = cartDao.getCartItems().first() | ||
| val currentItems = currentEntity.map { it.toCartItem() } | ||
| val cart = Cart(currentItems) | ||
|
|
||
| val updateCart = cart.plusProduct(product, quantity) | ||
| val updateCartItem = updateCart.cartItems.find { it.hasProduct(product) } ?: throw IllegalArgumentException("상품을 찾을 수 없습니다") | ||
| cartDao.insertOrUpdate(updateCartItem.toEntity()) | ||
| } | ||
|
|
||
| override suspend fun decreaseCartItem( | ||
| product: Product, | ||
| quantity: Quantity, | ||
| ) { | ||
| val currentEntity = cartDao.getCartItems().first() | ||
| val currentItems = currentEntity.map { it.toCartItem() } | ||
| val cart = Cart(currentItems) | ||
|
|
||
| val updateCart = cart.minusProduct(product, quantity) | ||
| val updateCartItem = updateCart.cartItems.find { it.hasProduct(product) } | ||
|
|
||
| if (updateCartItem == null) { | ||
| cartDao.deleteById(product.id) | ||
| } else { | ||
| cartDao.insertOrUpdate(updateCartItem.toEntity()) | ||
| } |
There was a problem hiding this comment.
장바구니 수량 갱신이 원자적이지 않습니다.
Line 24-30과 Line 37-48은 둘 다 현재 상태를 읽은 뒤 메모리에서 계산하고 다시 저장하는 방식입니다. 빠른 연타나 겹치는 코루틴 호출이 동시에 들어오면 같은 스냅샷을 기준으로 계산해서 수량 1회분이 유실될 수 있습니다. 이 경로는 트랜잭션이나 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/data/repository/CartRepositoryImpl.kt`
around lines 20 - 48, The addCartItem and decreaseCartItem implementations read
current state via cartDao.getCartItems(), compute a new Cart in memory (using
Cart.plusProduct and Cart.minusProduct) and then write back with
cartDao.insertOrUpdate/deleteById, which is not atomic and can lose updates
under concurrent calls; fix by making the update atomic—either wrap the
read-modify-write in a database transaction provided by your DAO/DB layer or add
atomic DAO operations (e.g., increment/decrement quantity by product id or
upsert-with-compare) and use those instead of getCartItems()+insertOrUpdate;
update addCartItem, decreaseCartItem to call the transactional/atomic DAO
methods (or use the DAO’s transaction block) so concurrent coroutines cannot
overwrite each other.
Comment on lines
+66
to
+71
| Text( | ||
| text = title, | ||
| fontSize = 12.sp, | ||
| fontWeight = W700, | ||
| overflow = TextOverflow.Ellipsis, | ||
| ) |
There was a problem hiding this comment.
maxLines 속성을 추가하는 것을 고려해보세요.
현재 TextOverflow.Ellipsis가 설정되어 있지만 maxLines가 없어 텍스트가 여러 줄로 래핑될 수 있습니다. 상품 타이틀이 길 경우 아이템 레이아웃이 불균형해질 수 있습니다.
SingleProductItem의 타이틀 텍스트(Line 70)에서 maxLines = 1을 사용한 것처럼, 이 컴포넌트에서도 동일한 제약이 필요한지 검토해보시기 바랍니다.
🤖 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/productlist/RecentProductList.kt`
around lines 66 - 71, The Text composable in RecentProductList.kt uses
TextOverflow.Ellipsis but lacks a maxLines constraint, which allows long titles
to wrap and break layout; update the Text call that renders title (the Text with
fontSize = 12.sp and fontWeight = W700) to include maxLines = 1 (same behavior
as SingleProductItem's title) so long product titles are truncated on a single
line with an ellipsis.
Comment on lines
+66
to
+75
| private fun loadInitData() { | ||
| viewModelScope.launch { | ||
| try { | ||
| allProducts = productRepository.getProducts() | ||
| fetchProducts() | ||
| } catch (e: Exception) { | ||
| println("ProductListViewModel Exception: $e.message") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
상품 로딩 실패 시 사용자 피드백이 부족합니다.
72번 라인에서 예외를 println으로만 처리하고 있습니다. 네트워크 오류 등으로 상품 목록을 불러오지 못했을 때 사용자는 빈 화면만 보게 됩니다.
다음을 생각해보세요:
ProductListUiState에 로딩 상태나 에러 상태를 추가하면 어떨까요?- 사용자에게 재시도 버튼을 제공하는 것은 어떨까요?
- ProductDetailViewModel의
isError플래그처럼 에러를 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/ui/productlist/viewmodel/ProductListViewModel.kt`
around lines 66 - 75, Replace the println exception handling in loadInitData()
with proper UI state updates: add/loading and error fields to ProductListUiState
(e.g., isLoading: Boolean, isError: Boolean, errorMessage: String?), set
isLoading=true before the repository call and false in finally, and on catch set
isError=true and errorMessage to e.message (or a user-friendly string); expose
the updated state (StateFlow/LiveData) to the UI and add a public retry method
(e.g., retryLoad() that calls loadInitData() or fetchProducts()) so the UI can
show an error view with a retry button—mirroring ProductDetailViewModel’s
isError approach and updating fetchProducts()/loadInitData() to emit these
states.
Comment on lines
+180
to
+184
| override suspend fun deleteCartItem(productId: String) { | ||
| cartItems.value = cartItems.value.filter { | ||
| it.hasProductId(productId) | ||
| } | ||
| } |
There was a problem hiding this comment.
deleteCartItem의 필터 로직을 다시 확인해보세요.
182번 라인의 filter 조건을 살펴보세요:
cartItems.value = cartItems.value.filter {
it.hasProductId(productId)
}다음을 생각해보세요:
filter는 조건이true인 항목을 남기는 함수입니다- 이 코드는 삭제하려는 상품을 남기고 있나요, 제거하고 있나요?
deleteCartItem의 의도와 실제 동작이 일치하나요?
조건식을 수정하면 어떻게 될까요?
🤖 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/ui/productlist/ProductListViewModelTest.kt`
around lines 180 - 184, The deleteCartItem implementation currently keeps items
matching productId because cartItems.value = cartItems.value.filter {
it.hasProductId(productId) } uses filter to retain true matches; update the
predicate to exclude the target product by negating the check (use
!it.hasProductId(productId)) so cartItems.value contains all items except the
one to delete in the deleteCartItem suspend function, referencing
cartItems.value and the hasProductId method.
Comment on lines
+76
to
+79
| - [x] Cart에 상품을 존재한다면 true를 반환한다 | ||
| - [x] Cart에 상품을 존재하지 않는다면 false를 반환한다 | ||
| - [x] Cart에 입력받은 상품의 Quantity를 반환한다 | ||
| - [x] Cart에 입력받은 상품이 없다면 null을 반환한다 |
There was a problem hiding this comment.
문법 오류를 수정하세요.
단위 테스트 설명에 조사(particle) 사용 오류가 있습니다:
- 76번째 줄: "Cart에 상품을 존재한다면" → "Cart에 상품이 존재한다면" (존재하다는 자동사이므로 주격 조사 '이' 사용)
- 77번째 줄: "Cart에 상품을 존재하지" → "Cart에 상품이 존재하지"
- 78-79번째 줄: "Cart에 입력받은" → "Cart에서 입력받은" (장소로부터 받는 의미이므로 '에서' 사용)
📝 수정 제안
-- [x] Cart에 상품을 존재한다면 true를 반환한다
-- [x] Cart에 상품을 존재하지 않는다면 false를 반환한다
-- [x] Cart에 입력받은 상품의 Quantity를 반환한다
-- [x] Cart에 입력받은 상품이 없다면 null을 반환한다
+- [x] Cart에 상품이 존재한다면 true를 반환한다
+- [x] Cart에 상품이 존재하지 않는다면 false를 반환한다
+- [x] Cart에서 입력받은 상품의 Quantity를 반환한다
+- [x] Cart에서 입력받은 상품이 없다면 null을 반환한다🤖 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 `@README.md` around lines 76 - 79, Fix the Korean particles in the README test
descriptions: change "Cart에 상품을 존재한다면" to "Cart에 상품이 존재한다면", "Cart에 상품을 존재하지
않는다면" to "Cart에 상품이 존재하지 않는다면", and update both occurrences of "Cart에 입력받은" to
"Cart에서 입력받은" so the phrases use the correct subject particle '이' for existence
and the location particle '에서' for receiving; update the four checklist lines
accordingly.
Comment on lines
+80
to
+85
| - [x] 새로운 상품을 추가하면 Cart에 새로운 상품이 추가된 Cart를 반환한다 | ||
| - [x] 이미 존재하는 상품을 추가하면 수량이 합쳐진 Cart 를 반환한다 | ||
| - [x] 삭제 수량이 보유 수량과 같으면 해당 상품이 제거된 Cart 를 반환한다 | ||
| - [x] 삭제 수량이 보유 수량보다 적으면 수량이 줄어든 Cart 를 반환한다 | ||
| - [x] 삭제 수량이 보유 수량보다 많으면 예외를 발생시킨다 | ||
| - [x] Cart에 없는 상품을 삭제하려고하면 예외를 발생시킨다 |
There was a problem hiding this comment.
조사 앞 공백을 제거하세요.
한글 문법 규칙에 따라 조사(를, 을 등)는 앞 단어와 붙여 써야 합니다. 현재 "Cart 를"처럼 공백이 포함되어 있습니다.
📝 수정 제안
-- [x] 새로운 상품을 추가하면 Cart에 새로운 상품이 추가된 Cart를 반환한다
-- [x] 이미 존재하는 상품을 추가하면 수량이 합쳐진 Cart 를 반환한다
-- [x] 삭제 수량이 보유 수량과 같으면 해당 상품이 제거된 Cart 를 반환한다
-- [x] 삭제 수량이 보유 수량보다 적으면 수량이 줄어든 Cart 를 반환한다
+- [x] 새로운 상품을 추가하면 Cart에 새로운 상품이 추가된 Cart를 반환한다
+- [x] 이미 존재하는 상품을 추가하면 수량이 합쳐진 Cart를 반환한다
+- [x] 삭제 수량이 보유 수량과 같으면 해당 상품이 제거된 Cart를 반환한다
+- [x] 삭제 수량이 보유 수량보다 적으면 수량이 줄어든 Cart를 반환한다📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [x] 새로운 상품을 추가하면 Cart에 새로운 상품이 추가된 Cart를 반환한다 | |
| - [x] 이미 존재하는 상품을 추가하면 수량이 합쳐진 Cart 를 반환한다 | |
| - [x] 삭제 수량이 보유 수량과 같으면 해당 상품이 제거된 Cart 를 반환한다 | |
| - [x] 삭제 수량이 보유 수량보다 적으면 수량이 줄어든 Cart 를 반환한다 | |
| - [x] 삭제 수량이 보유 수량보다 많으면 예외를 발생시킨다 | |
| - [x] Cart에 없는 상품을 삭제하려고하면 예외를 발생시킨다 | |
| - [x] 새로운 상품을 추가하면 Cart에 새로운 상품이 추가된 Cart를 반환한다 | |
| - [x] 이미 존재하는 상품을 추가하면 수량이 합쳐진 Cart를 반환한다 | |
| - [x] 삭제 수량이 보유 수량과 같으면 해당 상품이 제거된 Cart를 반환한다 | |
| - [x] 삭제 수량이 보유 수량보다 적으면 수량이 줄어든 Cart를 반환한다 | |
| - [x] 삭제 수량이 보유 수량보다 많으면 예외를 발생시킨다 | |
| - [x] 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 `@README.md` around lines 80 - 85, Remove the stray spaces before Korean
particles in the checklist lines so particles are attached to the preceding word
(e.g. change "Cart 를" to "Cart를", "Cart 를 반환한다" -> "Cart를 반환한다"); scan the
README.md checklist items for other occurrences of space-before-particle
patterns (를, 을, 은, 는, 에, 의, 로, 와/과 등) and correct them so each particle is
directly appended to the prior token.
byunghyunkim0
commented
May 11, 2026
Comment on lines
+10
to
17
| val MOCK_PRODUCTS_LIST: List<ProductResponse> = (1..35).map { i -> | ||
| ProductResponse( | ||
| id = i, | ||
| name = "품목$i", | ||
| price = Money(i * 1_000), | ||
| price = (i * 1_000), | ||
| imageUrl = "$IMAGE_BASE_URL${(i - 1) % 5}$IMAGE_URL_SUFFIX", | ||
| id = "$i", | ||
| ) | ||
| } |
Author
There was a problem hiding this comment.
서버에서 데이터를 보낼 때 도메인(Product)가 아닌 DB에서 찾은 값이라고 생각해서 ProductResponse로 변경했습니다
There was a problem hiding this comment.
좋은 판단입니다.
지금 코드에는 세 가지 모델이 명확히 분리되어 있죠.
Domain : Product, Cart, CartItem, Money, Quantity — 이 앱이 풀려는 문제
Network : ProductResponse — 서버가 우리에게 주는 형태
Local : CartItemEntity, RecentProductEntity — 디스크에 두기 좋은 형태
이 셋을 분리하는 이유는 “셋이 변할 이유가 다르기 때문”이에요.
서버가 갑자기 price를 Int가 아닌 String("1,200원")으로 내려준다고 하면,
Product(도메인)까지 같이 흔들려야 할까요?
Room 마이그레이션 때문에 컬럼 하나 추가해야 하면,
ViewModel 코드도 같이 흔들려야 할까요?
지금 구조에선 흔들리는 건 “경계의 매퍼 한 줄”뿐이고 Domain은 가만히 있어요. 그래서 좋은 결정이에요.
작은 확장 질문 하나만.
지금 toCartItem / toEntity / toProduct 매퍼가 모두 Repository 안 private extension으로 있어요.
Repository가 매퍼까지 들고 있는 게 자연스러울까요?
별도 파일/네임스페이스(예: data/local/mapper/CartItemMapper.kt)가 자연스러울까요?
절대적인 정답은 없어요 본인 의견 한번 내보시면 좋겠습니다.
byunghyunkim0
commented
May 11, 2026
Comment on lines
+24
to
+29
| val currentEntity = cartDao.getCartItems().first() | ||
| val currentItems = currentEntity.map { it.toCartItem() } | ||
| val cart = Cart(currentItems) | ||
|
|
||
| val updateCart = cart.plusProduct(product, quantity) | ||
| val updateCartItem = updateCart.cartItems.find { it.hasProduct(product) } ?: throw IllegalArgumentException("상품을 찾을 수 없습니다") |
Author
There was a problem hiding this comment.
CartRepository를 만들면서 도메인 로직을 어디서 수행해야할지 고민이 많았습니다.
viewModel에서 Cart 비즈니스 로직을 사용하면 DB에서 가져온 값들을 Cart로 만들어서 비즈니스 로직을 수행해야되서 Repository에 Cart를 넣었는데 이 방식이 더 자연스러운가요??
There was a problem hiding this comment.
본문에 남겨주신
Repository에 Cart를 넣었는데 이 방식이 더 자연스러운가요??
질문을 여기서 같이 짚어볼게요.
지금 addCartItem은
현재 카트 전체 read → Cart 도메인 생성 → cart.plusProduct(...) → 결과 중 한 줄 write
순서로 흐릅니다. 즉 Repository가 “저장소” 외에 Cart의 비즈니스 규칙 실행자 역할도 같이 하고 있는 셈이에요. 그래서 Repository 인터페이스로 추상화했음에도 Mock을 만들 때 어렵게 느끼셨던 거고요
가짜 Repository도 같은 비즈니스 규칙을 다시 들고 있어야 하니까요.
한 가지 시나리오를 떠올려보시면 좋겠습니다.
만약 다음 미션에서 카트가 DB뿐 아니라 서버에도 동시에 sync 되어야 한다고 할게요.
그렇다면 cart.plusProduct(...) 호출은 이 코드베이스 안에서 몇 번 등장해야 자연스러울까요?
DB Repository 안에 한 번, 서버 Repository 안에 한 번, 총 두 번 등장한다면 어떤 느낌이 드시나요?
그리고 거꾸로
cart.plusProduct(...)가 ViewModel/UseCase 쪽에서 한 번만 일어나고,
Repository는 그 결과만 받아 저장한다면 그때 Repository의 메서드 시그니처는 어떻게 바뀔까요?
(예: addCartItem(product, quantity) vs save(cart) vs updateQuantity(productId, newQuantity))
참고로 Guide to app architecture - Data layer와 Architecture: domain layer에 “Repository는 무엇까지 책임지는가”에 대한 안드로이드 팀의 그림이 있어요. 정답이라기보단, 본인이 그렸던 그림과 비교해보시면 좋겠습니다.
"자연스러운가요?" 라고 질문을 주신 이유도 본인이 생각했을 때 확신이 없기에 그렇다고 생각을 하는데 한 번 위 내용들을 정리하면서 확신을 가져봅시다.
byunghyunkim0
commented
May 11, 2026
Comment on lines
+147
to
+165
| class MockCartRepository : CartRepository { | ||
| private val cartItems = MutableStateFlow<List<CartItem>>(emptyList()) | ||
|
|
||
| override fun getCart(): Flow<List<CartItem>> = cartItems | ||
|
|
||
| override suspend fun addCartItem( | ||
| product: Product, | ||
| quantity: Quantity, | ||
| ) { | ||
| val findItem = cartItems.value.find { it.hasProduct(product) } | ||
|
|
||
| if (findItem != null) { | ||
| cartItems.value = cartItems.value.map { | ||
| if (it.hasProduct(product)) it.increase(quantity) else it | ||
| } | ||
| } else { | ||
| cartItems.value += CartItem(product, quantity) | ||
| } | ||
| } |
Author
There was a problem hiding this comment.
이 부분 때문에 Repository를 제대로 사용했는지 의문이 들었습니다.
interface를 사용했기 때문에 Mock을 만드는데 쉬울줄 알았습니다.
그런데 repository 안에 도메인 로직을 통해 데이터를 변경 시키니깐 Mock 객체를 만드는데 어려움이 느껴졌습니다.
Mock 로직을 repository 구현체의 로직과 똑같이 만들어서 테스트를 진행하는 방식이 자연스러운 건가요???🤔
There was a problem hiding this comment.
좋은 질문이네요
Mock 로직을 repository 구현체의 로직과 똑같이 만들어서 테스트를 진행하는 방식이 자연스러운 건가요???
그게 자연스럽게 느껴진다면, 추상화 경계를 잘못 잡았다는 신호에 가깝지 않을까요?
- 인터페이스가 잘 잡혀 있다면, Mock은 ‘저장만 흉내내는’ 단순한 in-memory 구현이면 끝나야 해요.
예: 가짜 Map<String, CartItem> 한 줄.
- 그런데 지금 Mock은 find { it.hasProduct(...) } + map { it.increase(...) } 같은
도메인 규칙까지 같이 재현하고 있어요.
왜냐하면 실구현 CartRepositoryImpl이 plusProduct / minusProduct 같은
도메인 규칙을 같이 들고 있어서, Mock도 따라 해야 ViewModel 테스트가 의도대로 굴러가니까요.
즉
“Mock이 무거워졌다”
↕ 같은 말
“Repository 인터페이스가 너무 많은 약속을 하고 있다”
그러면 여기서 2가지 정도 질문을 드리고, 이것에 대한 답변을 하다 보면 자연스럽게 해결이 될 것 같네요.
- 만약 CartRepository의 책임이 ‘정해진 형태의 CartItem 목록을 저장/조회’만 한다면, Mock은 얼마나 짧아질 수 있을까요?
- 그렇게 짧아진 Mock에서도 ‘+1 / -1’ 시나리오 테스트는 여전히 의미 있나요? 그 테스트는 이제 ‘무엇을’ 검증하는 테스트가 될까요?
그래도 인터페이스를 분리해두고 Mock을 직접 짜본 덕분에 이 “이상한 느낌”을 캐치하셨고, 그 덕에 저도 리뷰하는데 있어서 수월했네요.
lee-ji-hoon
requested changes
May 11, 2026
lee-ji-hoon
left a comment
lee-ji-hoon
left a comment
There was a problem hiding this comment.
바드 안녕하세요. 코니입니다.
3,4단계 각 사이클을 본인 손으로 다 거쳐오신 흔적들이 보이네요.
PR 본문의 “repository 구현이 일부 이해되지 않았다”가 이번 리뷰의 핵심으로 잡고 길게 리뷰를 해두었으니 천천히 읽어보시기 바랍니다.
그리고 코드래빗이 한 리뷰도 한 번 씩 보면 좋을 것 같으니 반영을 하면 했다는 내용과 안했다면 왜 안했는지를 본인의 의견을 남겨보시기 바랍니다! 🫡
Comment on lines
+24
to
+29
| val currentEntity = cartDao.getCartItems().first() | ||
| val currentItems = currentEntity.map { it.toCartItem() } | ||
| val cart = Cart(currentItems) | ||
|
|
||
| val updateCart = cart.plusProduct(product, quantity) | ||
| val updateCartItem = updateCart.cartItems.find { it.hasProduct(product) } ?: throw IllegalArgumentException("상품을 찾을 수 없습니다") |
There was a problem hiding this comment.
본문에 남겨주신
Repository에 Cart를 넣었는데 이 방식이 더 자연스러운가요??
질문을 여기서 같이 짚어볼게요.
지금 addCartItem은
현재 카트 전체 read → Cart 도메인 생성 → cart.plusProduct(...) → 결과 중 한 줄 write
순서로 흐릅니다. 즉 Repository가 “저장소” 외에 Cart의 비즈니스 규칙 실행자 역할도 같이 하고 있는 셈이에요. 그래서 Repository 인터페이스로 추상화했음에도 Mock을 만들 때 어렵게 느끼셨던 거고요
가짜 Repository도 같은 비즈니스 규칙을 다시 들고 있어야 하니까요.
한 가지 시나리오를 떠올려보시면 좋겠습니다.
만약 다음 미션에서 카트가 DB뿐 아니라 서버에도 동시에 sync 되어야 한다고 할게요.
그렇다면 cart.plusProduct(...) 호출은 이 코드베이스 안에서 몇 번 등장해야 자연스러울까요?
DB Repository 안에 한 번, 서버 Repository 안에 한 번, 총 두 번 등장한다면 어떤 느낌이 드시나요?
그리고 거꾸로
cart.plusProduct(...)가 ViewModel/UseCase 쪽에서 한 번만 일어나고,
Repository는 그 결과만 받아 저장한다면 그때 Repository의 메서드 시그니처는 어떻게 바뀔까요?
(예: addCartItem(product, quantity) vs save(cart) vs updateQuantity(productId, newQuantity))
참고로 Guide to app architecture - Data layer와 Architecture: domain layer에 “Repository는 무엇까지 책임지는가”에 대한 안드로이드 팀의 그림이 있어요. 정답이라기보단, 본인이 그렸던 그림과 비교해보시면 좋겠습니다.
"자연스러운가요?" 라고 질문을 주신 이유도 본인이 생각했을 때 확신이 없기에 그렇다고 생각을 하는데 한 번 위 내용들을 정리하면서 확신을 가져봅시다.
Comment on lines
+10
to
17
| val MOCK_PRODUCTS_LIST: List<ProductResponse> = (1..35).map { i -> | ||
| ProductResponse( | ||
| id = i, | ||
| name = "품목$i", | ||
| price = Money(i * 1_000), | ||
| price = (i * 1_000), | ||
| imageUrl = "$IMAGE_BASE_URL${(i - 1) % 5}$IMAGE_URL_SUFFIX", | ||
| id = "$i", | ||
| ) | ||
| } |
There was a problem hiding this comment.
좋은 판단입니다.
지금 코드에는 세 가지 모델이 명확히 분리되어 있죠.
Domain : Product, Cart, CartItem, Money, Quantity — 이 앱이 풀려는 문제
Network : ProductResponse — 서버가 우리에게 주는 형태
Local : CartItemEntity, RecentProductEntity — 디스크에 두기 좋은 형태
이 셋을 분리하는 이유는 “셋이 변할 이유가 다르기 때문”이에요.
서버가 갑자기 price를 Int가 아닌 String("1,200원")으로 내려준다고 하면,
Product(도메인)까지 같이 흔들려야 할까요?
Room 마이그레이션 때문에 컬럼 하나 추가해야 하면,
ViewModel 코드도 같이 흔들려야 할까요?
지금 구조에선 흔들리는 건 “경계의 매퍼 한 줄”뿐이고 Domain은 가만히 있어요. 그래서 좋은 결정이에요.
작은 확장 질문 하나만.
지금 toCartItem / toEntity / toProduct 매퍼가 모두 Repository 안 private extension으로 있어요.
Repository가 매퍼까지 들고 있는 게 자연스러울까요?
별도 파일/네임스페이스(예: data/local/mapper/CartItemMapper.kt)가 자연스러울까요?
절대적인 정답은 없어요 본인 의견 한번 내보시면 좋겠습니다.
Comment on lines
+147
to
+165
| class MockCartRepository : CartRepository { | ||
| private val cartItems = MutableStateFlow<List<CartItem>>(emptyList()) | ||
|
|
||
| override fun getCart(): Flow<List<CartItem>> = cartItems | ||
|
|
||
| override suspend fun addCartItem( | ||
| product: Product, | ||
| quantity: Quantity, | ||
| ) { | ||
| val findItem = cartItems.value.find { it.hasProduct(product) } | ||
|
|
||
| if (findItem != null) { | ||
| cartItems.value = cartItems.value.map { | ||
| if (it.hasProduct(product)) it.increase(quantity) else it | ||
| } | ||
| } else { | ||
| cartItems.value += CartItem(product, quantity) | ||
| } | ||
| } |
There was a problem hiding this comment.
좋은 질문이네요
Mock 로직을 repository 구현체의 로직과 똑같이 만들어서 테스트를 진행하는 방식이 자연스러운 건가요???
그게 자연스럽게 느껴진다면, 추상화 경계를 잘못 잡았다는 신호에 가깝지 않을까요?
- 인터페이스가 잘 잡혀 있다면, Mock은 ‘저장만 흉내내는’ 단순한 in-memory 구현이면 끝나야 해요.
예: 가짜 Map<String, CartItem> 한 줄.
- 그런데 지금 Mock은 find { it.hasProduct(...) } + map { it.increase(...) } 같은
도메인 규칙까지 같이 재현하고 있어요.
왜냐하면 실구현 CartRepositoryImpl이 plusProduct / minusProduct 같은
도메인 규칙을 같이 들고 있어서, Mock도 따라 해야 ViewModel 테스트가 의도대로 굴러가니까요.
즉
“Mock이 무거워졌다”
↕ 같은 말
“Repository 인터페이스가 너무 많은 약속을 하고 있다”
그러면 여기서 2가지 정도 질문을 드리고, 이것에 대한 답변을 하다 보면 자연스럽게 해결이 될 것 같네요.
- 만약 CartRepository의 책임이 ‘정해진 형태의 CartItem 목록을 저장/조회’만 한다면, Mock은 얼마나 짧아질 수 있을까요?
- 그렇게 짧아진 Mock에서도 ‘+1 / -1’ 시나리오 테스트는 여전히 의미 있나요? 그 테스트는 이제 ‘무엇을’ 검증하는 테스트가 될까요?
그래도 인터페이스를 분리해두고 Mock을 직접 짜본 덕분에 이 “이상한 느낌”을 캐치하셨고, 그 덕에 저도 리뷰하는데 있어서 수월했네요.
There was a problem hiding this comment.
ViewModel 안에 살아있는 상태가 네 군데예요.
_uiState / allProducts / _products / cart
‘현재까지 몇 개를 보여줬는가’가 반드시 _products 라는 mutable 리스트로만 표현되는 정보인가요?
| allProducts = productRepository.getProducts() | ||
| fetchProducts() | ||
| } catch (e: Exception) { | ||
| println("ProductListViewModel Exception: $e.message") |
There was a problem hiding this comment.
- println 인 점
- $e.message 는 실제로 무엇이 찍힐까요?
Comment on lines
+116
to
+124
| val Factory: ViewModelProvider.Factory = viewModelFactory { | ||
| initializer { | ||
| val context = this[ViewModelProvider.AndroidViewModelFactory.APPLICATION_KEY]!! | ||
| val database = AppDatabase.getDatabase(context) | ||
|
|
||
| val cartRepository = CartRepositoryImpl(database.cartDao()) | ||
| CartViewModel(cartRepository) | ||
| } | ||
| } |
There was a problem hiding this comment.
여기 포함 다른 ViewModel Factory가 모두 Repository를 직접 new로 만들고 있어요.
만약 Repository에 메모리 캐시가 생긴다면, 그 캐시는 화면마다 어떻게 될까요?
| <uses-permission android:name="android.permission.INTERNET" /> | ||
|
|
||
| <application | ||
| android:usesCleartextTraffic="true" |
There was a problem hiding this comment.
이거는 참고해보시기 바랍니다. 지금 디버그 빌드를 했다면 이 옵션이 됐어야 했을텐데 왜 킨 것인지도 궁금하네요.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
코드 리뷰 커뮤니케이션
5.10 첫 리뷰 (도메인 로직)
5.10 첫 리뷰 (UI 로직)
5.10 첫 리뷰 (viewModel + Repository + data)
📌 GitHub에서 PR에 댓글 남기는 방법
참고 자료
graph TD subgraph UI_Layer [UI Layer] Screens[Compose Screens] end subgraph Presentation_Layer [Presentation Layer] VM[ViewModels] UDF[Unidirectional Data Flow] end subgraph Domain_Layer [Domain Layer] Repo[Repositories] Models[Domain Models] end subgraph Data_Layer [Data Layer] Remote[Remote API Source] Local[Room Database] end %% Flow Screens <-->|State / Events| VM VM <-->|Request / Domain Data| Repo Repo <-->|Fetch / Sync| Remote Repo <-->|CRUD / Flow| Local Repo --- Models스크린샷