[3단계 - 상태 관리 | 4단계 - HTTP Client] 하로 미션 제출합니다 #174
Conversation
Walkthrough이 PR은 쇼핑 카트 애플리케이션을 단계 3-4로 진행하여 상태 관리 및 HTTP 클라이언트 기능을 구현합니다. 핵심 변경사항:
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ 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: 4
Note
Due to the large number of review comments, Critical 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/features/productDetail/ProductDetailScreen.kt (1)
310-319:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreview에서 기본
viewModel()호출 시 동작 확인 필요
ProductDetailScreen의viewModel파라미터가 기본값viewModel()을 가지는데,ProductDetailViewModel은ParcelProduct/CartRepository/RecentProductRepository를 요구합니다. Preview에서 별도 Factory 없이 호출되면 인스턴스화에 실패할 가능성이 큽니다.검토해 보실 점:
- Preview에서 실제 IDE Preview가 렌더링되나요? 아니면 빌드만 통과하는 상태인가요?
- ViewModel을 인자로 받는 화면을 Preview하기 위한 일반적인 패턴(예: Stateless한 내부 컴포저블만 Preview하거나, fake/test factory를 주입하는 방법)을 적용할 수 있을지 고려해 보세요.
ProductDetailContent처럼 상태/콜백을 받는 stateless 컴포저블을 Preview 전용으로 분리하면 어떤 이점이 있을지 한 번 정리해 보시는 것을 추천드립니다.🤖 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/features/productDetail/ProductDetailScreen.kt` around lines 310 - 319, The ProductDetailScreen's Preview fails because its default parameter uses viewModel() (ProductDetailViewModel) which requires real dependencies; update the Preview to avoid instantiating the real ViewModel by either (1) creating and calling a stateless composable (e.g., ProductDetailContent) with mocked state and no real VM/callbacks, or (2) injecting a fake/test ProductDetailViewModel instance into ProductDetailScreen from the Preview (avoid calling viewModel() there); locate references to ProductDetailScreen(viewModel()...), ProductDetailViewModel, and ProductDetailContent and implement one of these approaches so the Preview renders without needing the real repositories or ParcelProduct.
🟠 Major comments (22)
app/src/main/java/woowacourse/shopping/domain/cart/model/CartItem.kt-11-13 (1)
11-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick win수량 증감 API에 입력 경계 검증이 필요합니다.
Line 11, Line 13에서
targetQuantity가 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/domain/cart/model/CartItem.kt` around lines 11 - 13, The increaseQuantity and decreaseQuantity methods must validate targetQuantity is positive and prevent invalid post-change quantities: in increaseQuantity(targetQuantity: Int) check targetQuantity > 0 and throw an IllegalArgumentException (or domain-specific exception) if not, then perform copy(quantity = CartItemQuantity(quantity.value + targetQuantity)); in decreaseQuantity(targetQuantity: Int) check targetQuantity > 0 and compute newQty = quantity.value - targetQuantity and ensure newQty >= 1 (or the domain minimum) before creating CartItemQuantity(newQty), otherwise throw an IllegalArgumentException (or domain-specific exception) with a clear message; update tests accordingly to assert these validations.app/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/product/ProductRepositoryMockImpl.kt-10-10 (1)
10-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
!!로 인한 비정상 종료 가능성이 있습니다.Line [10]에서 id 미존재 시 NPE로 바로 종료됩니다. 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/main/java/woowacourse/shopping/data/source/local/repositoryImpl/product/ProductRepositoryMockImpl.kt` at line 10, The mock implementation of getProduct in ProductRepositoryMockImpl uses the non-null assertion (!!) which will throw an NPE if the id is missing; change it to explicitly handle the not-found case by replacing the !! with a safe lookup and throwing a clear, handled exception (e.g., NoSuchElementException or a domain ProductNotFoundException) with a descriptive message including the id; update the implementation that calls MockData.products.find { it.id == id } to use the elvis operator (?:) to throw the chosen exception so callers receive an explicit, meaningful error instead of an NPE.app/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/product/ProductRepositoryMockImpl.kt-14-17 (1)
14-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick win페이지 범위 초과 시
subList예외가 발생할 수 있습니다.Line [17]은
page * pageSize가 리스트 크기를 넘으면IndexOutOfBoundsException이 납니다. 시작/끝 인덱스를 먼저 검증하고 범위 밖 요청은 빈 리스트로 처리해 주세요.🤖 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/source/local/repositoryImpl/product/ProductRepositoryMockImpl.kt` around lines 14 - 17, In getPagedProducts, avoid IndexOutOfBoundsException from MockData.products.subList by validating and clamping the start/end indices: compute start = page * pageSize and if start >= MockData.products.size or start < 0 return an empty list; otherwise compute end = minOf(MockData.products.size, (page + 1) * pageSize) and return MockData.products.subList(start, end). Ensure you reference getPagedProducts and MockData.products when making the change.app/src/main/java/woowacourse/shopping/domain/cart/model/CartItems.kt-12-14 (1)
12-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick win신규 상품 추가 분기에서 수량이 과증가할 수 있습니다.
Line [13]은 신규 항목일 때도
cartItem의 현재 수량에targetQuantity를 다시 더합니다. 입력cartItem.quantity가 이미 선택 수량이라면 최종 수량이 과하게 커질 수 있으니, “증가량”과 “목표수량” 의미를 한 가지로 통일해 주세요.🤖 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/model/CartItems.kt` around lines 12 - 14, The new-item branch in CartItems.add (using searchCartItem and CartItems(_value = value + cartItem.increaseQuantity(targetQuantity))) wrongly treats the incoming cartItem.quantity as a base and adds targetQuantity again, causing over-increment; change the logic so that when searchCartItem(cartItem) is false you append a cart item whose quantity is set to the targetQuantity (e.g. create/clone the cartItem with quantity = targetQuantity or call a setQuantity/withQuantity helper) instead of calling increaseQuantity; continue to use increaseQuantity only in the existing-item branch (the else that handles updating an existing CartItem).app/src/main/java/woowacourse/shopping/ShoppingApplication.kt-7-10 (1)
7-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMock 서버가 모든 빌드에서 실행됩니다.
확인된 문제:
ShoppingApplication.onCreate()에서ShoppingMockServer.start()를 무조건 호출하고 있으며,build.gradle에서mockwebserver3의존성이implementation()으로 설정되어 있어 릴리스 빌드에도 포함됩니다. 결과적으로 프로덕션 빌드에서도 Mock 서버가 실행되어 실제 데이터가 아닌 테스트 데이터를 사용하게 됩니다.이를 해결하기 위해 다음을 고려해보세요:
mockwebserver3의존성을debugImplementation()으로 변경하면 디버그 빌드에만 포함됩니다ShoppingApplication.onCreate()에서 조건부로 Mock 서버를 시작하려면 빌드 타입을 어떻게 구분할 수 있을까요? (BuildConfig.DEBUG활용)DataProvider.kt에서ShoppingMockServer.baseUrl을 무조건 사용하고 있는데, 환경에 따라 실제 서버 URL을 동적으로 선택하는 방식을 생각해보세요🤖 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/ShoppingApplication.kt` around lines 7 - 10, ShoppingMockServer is started unconditionally in ShoppingApplication.onCreate() and mockwebserver3 is currently included for all builds; change the Gradle dependency from implementation(...) to debugImplementation(...) so MockWebServer is only packaged in debug builds, then guard the call to ShoppingMockServer.start() in ShoppingApplication.onCreate() with a BuildConfig.DEBUG check (only start when true). Also update DataProvider (where ShoppingMockServer.baseUrl is used) to select the real remote base URL when BuildConfig.DEBUG is false (e.g., use ShoppingMockServer.baseUrl only when BuildConfig.DEBUG is true) so production builds use the real endpoint.app/src/main/java/woowacourse/shopping/domain/cart/model/Cart.kt-26-33 (1)
26-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick win페이지 범위 검증이 불완전합니다.
isPageValid(page)는 페이지가 0 이상인지만 확인하지만,fromIndex가 실제 리스트 크기를 초과하는 경우는 검증하지 않습니다.
CartItems.subList를 살펴보니 상한(toIndex)은minOf(toIndex, value.size)로 처리하지만, 하한(fromIndex)은 보호되지 않습니다. 따라서cartItems.subList(fromIndex, fromIndex + pageSize)를 호출할 때fromIndex >= value.size인 경우IndexOutOfBoundsException이 발생합니다.이 경우를 어떻게 처리해야 할지 고민해보세요.
힌트:
cartItems.size()와 요청된 페이지 범위를 비교해보세요- 페이지 크기를 초과하는 경우 어떻게 처리할지 결정해야 합니다 (예외 발생 vs. 빈 리스트 반환 vs. 가능한 만큼만 반환)
🤖 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/model/Cart.kt` around lines 26 - 33, The getPage function currently only checks isPageValid(page) (non-negative) but must also ensure the computed fromIndex = page * pageSize does not exceed cartItems.size; update getPage to compute fromIndex and if fromIndex >= cartItems.size return an empty List<CartItem> (or throw a clear exception if you prefer consistent failure), otherwise compute toIndex = minOf(fromIndex + pageSize, cartItems.size) and return cartItems.subList(fromIndex, toIndex); keep references to getPage, isPageValid, cartItems, fromIndex, and pageSize so reviewers can find the change.app/src/main/java/woowacourse/shopping/data/source/remote/ProductRemoteDataSource.kt-24-30 (1)
24-30:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift실패 응답을
emptyList()로 흡수하면 상위 계층이 장애를 구분할 수 없습니다.현재는 실패/본문 누락/정상 빈 목록이 모두 같은 결과라서, UI 에러 처리(토스트 등)와 캐시 정책이 왜곡되기 쉽습니다. 실패 신호를 별도로 전달하는 계약(예: 예외/Result 분리)로 정리하는 방향을 고려해 주세요.
🤖 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/source/remote/ProductRemoteDataSource.kt` around lines 24 - 30, ProductRemoteDataSource currently swallows failure cases by returning emptyList() for response.isSuccessful == false and when response.body is null; change this to surface distinct failure signals instead of an empty list by throwing a descriptive exception (e.g., IOException or a custom RemoteDataException) when !response.isSuccessful (include response.code() and/or response.message()) and when response.body == null, while keeping the successful branch that uses Json.decodeFromString(ListSerializer(ProductEntity.serializer()), responseBody); update callers (or change the method to return Result<List<ProductEntity>>) to handle these errors accordingly.app/src/main/java/woowacourse/shopping/data/source/remote/ProductRepositoryRemoteImpl.kt-11-15 (1)
11-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick win첫 조회 실패 결과까지 캐시되어 복구가 막힐 수 있습니다.
fetchAllProducts()가 첫 결과를 무조건 캐시해서, 일시적인 네트워크 실패로 빈 리스트를 받으면 이후에도 계속 빈 데이터만 반환할 가능성이 큽니다. 실패와 “정상 빈 데이터”를 구분해 캐시 전략을 분리해보는 게 좋겠습니다.🤖 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/source/remote/ProductRepositoryRemoteImpl.kt` around lines 11 - 15, fetchAllProducts currently unconditionally caches the first fetch into cachedProducts which can lock in a bad empty result; change fetchAllProducts to call remoteDataSource.getProducts() inside a try/catch, only assign cachedProducts = it when the call succeeds and the returned list is non-empty (or otherwise decide a success criterion), return the fetched list even if empty but do not overwrite cachedProducts on exceptions or on transient-empty responses, and leave cachedProducts untouched so future successful fetches can replace it; reference fetchAllProducts(), cachedProducts and remoteDataSource.getProducts() when making the change.app/src/main/java/woowacourse/shopping/data/NetworkMonitor.kt-26-32 (1)
26-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick win네트워크 손실 시 실제 연결 상태를 재확인해야 합니다.
onLost()콜백이 호출되었다고 해서 모든 네트워크 연결이 끊긴 것은 아닙니다. 여러 개의 네트워크가 동시에 활성화될 수 있는 Android 환경에서, WiFi가 끊겨도 모바일 데이터는 여전히 연결되어 있을 수 있습니다.현재처럼
onLost()에서 바로false를 emit하면, 실제로는 인터넷이 연결되어 있는데도 오프라인으로 표시되는 오탐이 발생할 수 있습니다. 대신onLost()콜백 내에서도isCurrentlyConnected()를 호출해서 현재 실제 연결 상태를 재평가한 후 emit하는 방식을 고려해보세요.🤖 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/NetworkMonitor.kt` around lines 26 - 32, onLost currently emits false unconditionally which can falsely mark the app offline; inside the NetworkMonitor's onLost(network: Network) callback call the existing isCurrentlyConnected() check and emit its boolean result via trySend(...) instead of trySend(false), so the emitted state reflects actual connectivity (use the same helper used elsewhere to reevaluate overall network status).app/src/main/AndroidManifest.xml-10-10 (1)
10-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
usesCleartextTraffic="true"를 전체 앱에 상시 적용하는 것은 보안 범위가 큽니다.Line 10 설정은 디버그 빌드뿐 아니라 릴리스 빌드에서도 모든 HTTP 평문 트래픽을 허용합니다. MockWebServer 테스트 목적이라면 네트워크 보안 설정(network security config) 파일로 디버그 환경에만 제한하는 것이 안전합니다. 현재 구성에서는 프로덕션 환경까지 평문 통신을 허용하므로 즉시 개선이 필요합니다.
🤖 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 10, 현재 AndroidManifest의 usesCleartextTraffic="true"을 전체 앱에 적용하고 있으므로 이를 제거하고 평문 트래픽 허용을 디버그 빌드로 한정하세요: manifest에서 usesCleartextTraffic 속성을 제거하고 대신 네트워크 보안 구성 파일(network_security_config.xml)을 만들어 디버그 전용으로 cleartext를 허용하도록 설정한 뒤, 이 구성은 디버그 전용 AndroidManifest(또는 debug 소스셋의 매니페스트 오버레이)에서만 android:networkSecurityConfig로 참조되게 하세요; 관련 식별자: usesCleartextTraffic, network_security_config.xml, debug AndroidManifest (debug build type 오버레이).app/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/recentProduct/RecentProductRepositoryRoomImpl.kt-20-24 (1)
20-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
catch (Exception)이 코루틴 취소까지 삼켜서 취소 전파가 깨질 수 있습니다.현재 구현은
CancellationException까지 흡수할 수 있어 취소 협력이 깨질 위험이 있습니다. 예상 가능한 예외만 좁혀 잡고, 취소 예외는 반드시 재전파하는 형태로 정리해 주세요.이 문제는 20-24번 줄과 31-35번 줄 모두에 해당합니다.
🤖 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/source/local/repositoryImpl/recentProduct/RecentProductRepositoryRoomImpl.kt` around lines 20 - 24, The current catch-all (catch (e: Exception)) around calls to productRepository.getProduct(entity.productId) swallows CancellationException and breaks coroutine cancellation; change these catches to rethrow cancellation: either use catch (e: CancellationException) { throw e } before handling other exceptions, or replace with catch (e: Exception) { if (e is CancellationException) throw e else /* existing fallback (return null or log) */ }; apply the same fix for both occurrences in RecentProductRepositoryRoomImpl where getProduct(entity.productId) is invoked.app/src/main/java/woowacourse/shopping/data/source/remote/ShoppingMockServer.kt-14-39 (1)
14-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick win백그라운드 스레드에서 서버가 시작되므로 초기 요청 타이밍 레이스 가능성이 있습니다.
start()메서드가 즉시 반환되지만,server.start()는 백그라운드 스레드에서 비동기로 실행됩니다. 앱이 빠르게 시작되는 경우,DataProvider의remoteDataSource가 초기화되어ShoppingMockServer.baseUrl에 접근할 때 서버가 아직 준비되지 않은 상태일 수 있습니다.서버 시작을 완료한 후 호출자에게 제어를 반환하도록 개선할 방법을 생각해 보세요. (예: 서버 준비를 알리는 메커니즘 추가)
🤖 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/source/remote/ShoppingMockServer.kt` around lines 14 - 39, start() currently spawns a background Thread and returns immediately, causing a race where ShoppingMockServer.baseUrl may be read before server.start() completes; make start() synchronous or add a readiness signal so callers block until the mock server is ready (e.g., remove the background Thread and call server.start() directly or use a CountDownLatch / CompletableFuture that the Dispatcher or thread sets after server.start() and have start() await it), update ShoppingMockServer.start() to wait for that readiness before returning and ensure any callers of ShoppingMockServer.baseUrl depend on that readiness mechanism.app/src/main/java/woowacourse/shopping/data/source/local/CartDatabase.kt-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
fallbackToDestructiveMigration()은 향후 스키마 업그레이드 시 사용자 데이터 손실을 초래할 수 있습니다.현재는 새로운 데이터베이스 생성이므로 즉각적인 문제가 없지만, 버전 2에서 3으로 업그레이드될 때 장바구니 및 최근 본 상품 데이터가 삭제될 것입니다. 지금부터 명시적
Migration클래스로 스키마 변경을 관리하는 것이 향후 데이터 손실을 방지하는 방법입니다. 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/data/source/local/CartDatabase.kt` at line 33, Replace fallbackToDestructiveMigration() with explicit Room Migration objects and register them via addMigrations(...) on the Room.databaseBuilder used in CartDatabase; create Migration instances (e.g., val MIGRATION_2_3 = object : Migration(2, 3) { override fun migrate(db: SupportSQLiteDatabase) { /* schema changes: ALTER TABLE / CREATE TABLE / COPY data as needed */ } }) in the CartDatabase companion or a dedicated migrations file, implement the SQL needed to preserve cart and recently viewed data, and pass those migrations into databaseBuilder.addMigrations(MIGRATION_2_3, /* other migrations */) so upgrades don't destructively delete user data.app/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/cart/CartRepositoryRoomImpl.kt-22-26 (1)
22-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick win예외를 삼키면 장바구니 누락 원인을 추적하기 어려워집니다.
Line 24에서
Exception을 그대로 삼키고 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 `@app/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/cart/CartRepositoryRoomImpl.kt` around lines 22 - 26, In CartRepositoryRoomImpl replace the blanket catch around productRepository.getProduct(...) that swallows Exception with targeted handling: catch only the expected "product not found" exception (e.g., NotFoundException or whatever your productRepository throws) and return null for that case, but for other exceptions log the error via the repository/logger (or rethrow) so parsing/network/data corruption errors are visible; ensure references to productRepository.getProduct and CartRepositoryRoomImpl are updated accordingly.app/src/main/java/woowacourse/shopping/data/DataProvider.kt-16-38 (1)
16-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick win싱글톤 캐시 경계에서
Activity컨텍스트가 보관될 수 있습니다.Line 16, 30, 36처럼 외부
context를 그대로 넘기면 호출자가Activity일 때 누수 위험이 생깁니다. DataProvider 진입점에서applicationContext로 정규화해두는 쪽을 고려해보실까요?🤖 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/DataProvider.kt` around lines 16 - 38, The DataProvider currently stores objects created with the raw Context passed into getNetworkMonitor, getCartRepository, and getRecentProductRepository which can leak an Activity; update these factory methods to normalize the incoming Context to applicationContext (e.g., use context.applicationContext) before constructing NetworkMonitor, calling CartDatabase.getDatabase, or passing into any long-lived singletons (networkMonitor, CartRepositoryRoomImpl, RecentProductRepositoryRoomImpl) so only the Application context is retained.app/src/main/java/woowacourse/shopping/features/productList/ProductListViewModel.kt-42-44 (1)
42-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
totalProductCount초기화 시점 때문에 첫 페이지에서isLastPage가 잘못 계산될 수 있습니다.
init에서 Line 43으로moreProducts()가 먼저 실행되는데, Line 96 계산 시점의totalProductCount는 아직 0입니다. 이러면isLastPage=true로 고정되어 더보기 버튼/페이징이 조기에 막힐 수 있습니다. 카운트 초기화 책임을moreProducts()경로로 옮길지 한번 점검해보실래요?Also applies to: 46-49, 68-97
🤖 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/features/productList/ProductListViewModel.kt` around lines 42 - 44, The init block calls moreProducts() before totalProductCount is initialized which causes isLastPage to be computed incorrectly; update the flow so totalProductCount is set before isLastPage is evaluated by either (a) initializing totalProductCount in init (set a sensible initial value or load the count first) and then calling moreProducts(), or (b) move the totalProductCount assignment and isLastPage calculation into the moreProducts() response handling so that after fetching the first page you assign totalProductCount and then compute isLastPage; adjust the logic in ProductListViewModel.kt (init, moreProducts(), and the isLastPage calculation site) so isLastPage is always computed only after totalProductCount is populated.app/src/main/java/woowacourse/shopping/features/cart/CartScreen.kt-62-62 (1)
62-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
CartViewModel의 생성자 의존성으로 인해 기본viewModel()경로가 작동하지 않습니다.CartViewModel은 CartRepository를 생성자에서 요구하는데(16-18줄), Line 62의 기본 매개변수
viewModel()은 이 의존성을 제공할 수 없습니다. 따라서 Line 340-348의 Preview는 명시적인 viewModel 인자를 제공하지 않으면서 기본값에 의존하게 되어 런타임에 실패합니다.이를 해결하기 위해 다음을 고려해보세요:
- CartScreen의 viewModel 매개변수에서 기본값을 제거하고 호출처에서 명시적으로 주입하는 방식
- 또는 Preview에서 테스트용 mock CartViewModel을 생성하여 전달하는 방식
🤖 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/features/cart/CartScreen.kt` at line 62, The CartScreen's default parameter viewModel: CartViewModel = viewModel() fails because CartViewModel requires a CartRepository in its constructor; remove the default so callers must pass an explicit CartViewModel or provide a test/mock in previews. Specifically, update CartScreen to require viewModel: CartViewModel (no default) and modify callers (including the Preview at the region around lines 340-348) to supply either the real injected viewModel or a mock/test instance, or alternatively create a mock CartViewModel inside the Preview and pass it into CartScreen to avoid runtime injection failures.app/src/main/java/woowacourse/shopping/features/cart/CartViewModel.kt-35-60 (1)
35-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
loadCartPage()의 코루틴 실행 구조를 다시 살펴봐주세요.
removeCartItem(),increaseCartItem(),decreaseCartItem()에서 각각viewModelScope.launch로 감싼 후loadCartPage()를 호출하고 있습니다.loadCartPage()자체도 내부에서 다시viewModelScope.launch를 사용합니다.몇 가지 생각해볼 점들입니다:
- 리포지토리 작업이 완료되고
loadCartPage()를 호출할 때, 이 호출이 즉시 반환되는데 내부의launch블록은 언제 실행되나요?- 여러 액션이 빠르게 연속으로 호출될 때(예: 수량 +/- 빠른 연타),
_uiState.update호출들이 어떤 순서로 실행될까요?- 현재 구조에서 UI 상태 갱신의 순서를 보장할 수 있을까요?
코루틴 스코프와
suspend함수의 역할을 생각해보면서, 같은 코루틴 컨텍스트에서 순차적으로 실행되게 구조화하는 방법을 찾아보세요.🤖 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/features/cart/CartViewModel.kt` around lines 35 - 60, loadCartPage() currently starts its own viewModelScope.launch while removeCartItem(), increaseCartItem(), and decreaseCartItem() also launch before calling it, causing nested asynchronous launches and unpredictable UI update ordering; change loadCartPage() to be a suspend function (remove its internal viewModelScope.launch) so callers can call it from their existing viewModelScope.launch blocks, ensuring repository calls and the subsequent _uiState.update run sequentially in the same coroutine context and preserve ordering.app/src/main/java/woowacourse/shopping/features/productList/ProductListScreen.kt-70-70 (1)
70-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ProductListScreen의 기본viewModel()호출은 의존성을 충족하지 못해 Preview와 기본값 경로에서 런타임 실패 위험이 있습니다.Line 70의 기본값
viewModel()을 보면,ProductListViewModel이 필요로 하는 4가지 의존성(NetworkMonitor, ProductRepository, CartRepository, RecentProductRepository)을 전달할 방법이 없습니다. Activity에서는ProductListViewModelFactory로 안전하게 주입되지만, Line 468~476 Preview는 기본값을 사용하므로 호출 시 런타임 오류가 발생합니다.해결 방향:
- Composable의 기본값 파라미터를 제거하거나,
- Preview와 테스트를 위해 mock/stub ViewModel을 제공하는 별도의 팩토리나 파라미터를 고려해보세요.
또한 이 패턴이 프로젝트의 다른 Composable에서도 반복되는지 확인하는 것도 좋은 습관입니다.
🤖 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/features/productList/ProductListScreen.kt` at line 70, The ProductListScreen's default parameter viewModel: ProductListViewModel = viewModel() will fail in Preview because its dependencies (NetworkMonitor, ProductRepository, CartRepository, RecentProductRepository) cannot be provided; update ProductListScreen so it no longer calls viewModel() as a default: either remove the default and make the viewModel parameter required, or add an explicit optional preview/test injection such as a previewViewModel factory parameter (e.g., accept a ProductListViewModelFactory or a PreviewProductListViewModel singleton) and use that for composable previews; ensure references to ProductListScreen and ProductListViewModel (and the existing ProductListViewModelFactory) are updated so Activity injection still works and previews/tests can supply a lightweight/mock view model.app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailViewModel.kt-22-32 (1)
22-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winViewModel 내부의 상태 중복 보관 검토
_uiState(MutableStateFlow)와는 별개로name,imageUrl,price,quantity,minusEnabled,isLatestProduct,latestProduct를 각각private var로 두고 양쪽을 동시에 갱신하고 있습니다. 이렇게 되면 “원천(single source of truth)”이 둘로 쪼개져서, 한쪽만 갱신했을 때 UI와 내부 도메인 상태가 어긋날 위험이 있습니다.스스로 점검해 보실 만한 질문:
- 어떤 정보가 “UI에 표시하기 위한 상태”이고, 어떤 정보가 “비즈니스 로직에만 필요한 상태”인가요? 후자가 정말로 별도 필드여야 하나요?
_uiState.value로부터 동일한 값을 즉시 읽을 수 있다면, 별도var백업이 굳이 필요할까요?- 단가/총가/수량처럼 파생 가능한 값은 저장하는 대신 계산해서 노출할 수 있지 않을까요?
상태가 한 곳에 모이면
increase/decrease,changeProduct등의 메서드도 단일update {}호출로 단순화되어 버그 가능성이 줄어듭니다.🤖 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/features/productDetail/ProductDetailViewModel.kt` around lines 22 - 32, The ViewModel currently duplicates state between _uiState (MutableStateFlow<ProductDetailUiState>) and private vars (name, imageUrl, price, quantity, minusEnabled, isLatestProduct, latestProduct); consolidate to a single source of truth by removing those separate private vars and always reading/writing from _uiState (use uiState.value for reads) and update state via _uiState.update { ... } in methods like increase, decrease, and changeProduct; compute derived values (e.g., total price, minusEnabled) on-demand in ProductDetailUiState or in getters rather than storing them as separate fields, and keep only domain-only data that truly cannot be exposed to UI (if any) as distinct properties.app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailScreen.kt-80-89 (1)
80-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
!!강제 언래핑과 상태 필드 이름 불일치 문제라인 84의
onLatestProductClick(uiState.latestProduct!!)는 NPE 위험이 있습니다. 호출 시점에!isLatestProduct조건이 만족되더라도 람다 캡처 시점과 실제 클릭 시점 사이에 상태가 바뀔 수 있고, 무엇보다 “isLastProduct == false⇒latestProduct != null”이라는 불변식은 ViewModel과의 암묵적 결합에만 의존합니다.또한 라인 83에서
uiState.isLastProduct를 로컬에서는isLatestProduct로 받고 있는데, 이는 ViewModel과 UiState의isLastProduct/isLatestProduct명명 혼란(파일 2 참고)을 화면 코드까지 끌고 옵니다.스스로 점검해 보실 만한 질문 몇 가지를 남깁니다:
- UI 모델 단계에서 “최근 본 상품 카드”가 의미적으로 단일 옵셔널 값(null이면 카드 없음)으로 표현되면,
if (!isLatestProduct)+!!패턴을 제거할 수 있지 않을까요?isLastProduct라는 이름이 “마지막(최후)의 상품”인지 “가장 최근에 본 상품인지 여부”인지 모호하지는 않나요? 어느 쪽 의미가 의도였는지 한 단어로 표현할 수 있을지 고민해 보세요.🤖 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/features/productDetail/ProductDetailScreen.kt` around lines 80 - 89, 현재 코드의 주요 문제는 uiState.latestProduct!!로 강제 언래핑해 NPE 위험을 만드는 점과 isLatestProduct/isLastProduct라는 불일치한 이름으로 의미가 혼란스러운 점입니다; 해결 방법은 ProductDetailScreen에서 uiState.latestProduct를 로컬 nullable 변수(e.g. val latestProduct = uiState.latestProduct)로 안전하게 캡처하고 onLatestProductClick에는 nullable을 허용하거나 클릭 람다 내부에서 latestProduct?.let { onLatestProductClick(it) }처럼 null 체크를 해 !!를 제거하고, UI/UiState 전체에서 isLastProduct 또는 isLatestProduct 중 하나의 명확한 이름으로 통일하여 ViewModel과 화면이 같은 의미를 사용하도록 변경하세요 (참고 심볼: uiState.latestProduct, onLatestProductClick, uiState.isLastProduct/isLatestProduct).app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailViewModel.kt-60-81 (1)
60-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
changeProduct의 강제 언래핑과 상태 의미 점검라인 61의
_uiState.value.latestProduct!!는 호출 시점에latestProduct가 null이면 즉시 크래시로 이어집니다. 또한 다음 의미적인 점도 함께 점검해 보시기 바랍니다.
- UI에서는
isLastProduct == false일 때만 카드가 보이고 그때만 이 함수가 호출되도록 막혀있지만, ViewModel은 호출자(UI)의 가정에 의존하는 형태입니다. 만약 다른 경로에서 이 메서드가 호출된다면 어떻게 방어할 수 있을까요?price = product.price.value한 줄은 “현재 표시 가격”을 단가로 리셋합니다. 그런데increaseCartItem/decreaseCartItem에서는price를 총액(누적가)처럼 다룹니다. 같은 필드가 시점에 따라 단가/총액으로 의미가 바뀌는 셈인데, 이 이중 의미는 어떻게 정리하면 좋을지 고민해 보세요(예: 단가는 상수처럼 두고, 총액은quantity * unitPrice로 매번 파생).NPE 회피는 옵셔널을 안전하게 처리하거나(
?.let { ... }) 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/features/productDetail/ProductDetailViewModel.kt` around lines 60 - 81, changeProduct currently force-unwraps _uiState.value.latestProduct with !! which can NPE if called unexpectedly; update changeProduct to safely guard against null (e.g., _uiState.value.latestProduct?.let { latest -> ... } or early return if latestProduct == null) and operate on that non-null local; also stop conflating unit price and aggregate price by keeping product.price as unitPrice (e.g., unitPrice = latest.price.value) and derive totalPrice as quantity * unitPrice when updating _uiState and when used in increaseCartItem/decreaseCartItem so price semantics are consistent across ProductDetailViewModel (refer to changeProduct, latestProduct, product.price, increaseCartItem/decreaseCartItem).
🟡 Minor comments (4)
README.md-74-75 (1)
74-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStateHolder 언급을 ViewModel/UDF 기준으로 최신화해주세요.
PR 설명에는 ViewModel 전환이 반영되어 있는데, README에는 여전히
StateHolder가 남아 있어 현재 구조를 오해할 수 있습니다.🤖 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 74 - 75, The README still refers to "StateHolder" which is outdated; update those references to "ViewModel" (or "UDF" where appropriate) and adjust the wording so it matches the PR's architecture: e.g., replace "StateHolder가 장바구니 repository를 알고 있어야 한다" with "ViewModel/UDF가 장바구니 repository에 접근해야 한다" and replace "각 상품이 장바구니에 몇 개가 담겨있는지 알고 있어야 한다" with "ViewModel/UDF가 각 상품의 장바구니 수량을 관리/노출해야 한다" so the README matches the ViewModel/UDF-based design described in the PR.README.md-47-47 (1)
47-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win문서 오탈자/맞춤법을 정리해주세요.
예:
메세지→메시지,변경되야→변경되어야,알고있다→알고 있다,어짜피→어차피.
README 신뢰도와 가독성에 바로 영향이 있습니다.Also applies to: 65-65, 76-76, 80-80
🤖 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` at line 47, Fix Korean spelling/spacing typos in the README: replace occurrences of "메세지" with "메시지", "변경되야" with "변경되어야", "알고있다" with "알고 있다", and "어짜피" with "어차피" (and apply the same corrections to the other affected occurrences referenced in the review); update the sentence "네트워크가 끊길 경우 Toast 메세지가 나타나게끔 기능을 구현했다." to use "메시지" and adjust spacing where needed so phrases like "알고 있다" are two words.app/src/test/java/woowacourse/shopping/cart/CartItemQuantityTest.kt-20-21 (1)
20-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win테스트 이름과 검증 대상이 어긋나 있습니다
Line 20의 테스트명은 “예외가 발생하지 않는다”인데, Line 21은 생성자 호출 없이
isValueValid만 확인하고 있습니다. 생성 성공(예외 미발생)을 직접 검증하도록 맞추는 편이 의도를 더 정확히 보장합니다.🤖 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/cart/CartItemQuantityTest.kt` around lines 20 - 21, The test name says "no exception is thrown" but it only calls CartItemQuantity.isValueValid(value); update the test to actually construct a CartItemQuantity with the provided value (e.g., call the CartItemQuantity constructor or factory used in production) to assert that creation succeeds without throwing, or alternatively rename the test to reflect that it only validates the value; target the CartItemQuantity constructor/newInstance and the isValueValid method so the test either constructs CartItemQuantity(value) and asserts no exception, or changes the test name to match the current behavior.app/src/main/java/woowacourse/shopping/data/source/remote/ProductEntity.kt-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProductEntity의
id필드에 빈 문자열 기본값이 적절한지 검토하세요Line 11의
id: String = ""기본값을 고려할 때, API 응답에서id필드가 누락되면 어떻게 되는지 생각해보세요. 현재 설정에서는toDomain()메서드를 통해 빈 문자열이 그대로 Product 도메인 객체의 id로 전달됩니다.다음 질문들을 자문해보세요:
id는 정말 선택 사항입니까?- 빈 문자열 id로
getProduct(id: String), 최근 본 상품 매핑, 장바구니 조회 등의 작업이 제대로 작동할까요?- ProductName처럼 id도 유효성 검사가 필요하지 않을까요?
ProductName의 검증 방식(비어있으면 예외)을 참고하여, id 필드가 API 계약에서 필수라면 이를 반영하도록 수정하는 방법을 생각해보세요. 필드를 선택 사항으로 유지할지, 기본값을 제거할지, 아니면 도메인 계층에서 유효성 검사를 추가할지 결정해야 합니다.
🤖 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/source/remote/ProductEntity.kt` at line 11, ProductEntity currently gives id a default empty string which lets missing API ids silently become invalid domain ids; update ProductEntity.id handling so missing ids are caught: either remove the default so id is required in the DTO (make it non-optional/no default) or make id nullable (String?) and add a validation in ProductEntity.toDomain() that throws a clear exception if id is null/blank (similar to ProductName validation), then map a validated id into the Product domain; locate ProductEntity and its toDomain() to apply the chosen change and ensure callers/tests reflect the stricter contract.
🧹 Nitpick comments (10)
README.md (1)
76-80: ⚡ Quick win질문형 메모를 확정된 요구사항 문장으로 정리하는 것을 권장합니다.
현재 README에 회의 메모 톤(질문형/말줄임표/빈 bullet)이 섞여 있어, 스펙 문서로 읽기 어렵습니다. 결정된 규칙(예: 비교 키, 최근 본 상품 10개 유지 정책)을 단정형으로 고정해 주세요.
Also applies to: 129-130
🤖 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 - 80, Convert the question-style bullets in the README (the entries starting with “상품 목록 화면에서도 장바구니에 담겨있는 상품들을 알고있다..?” and the related bullets about comparing ids) into definitive requirement statements: state that product listing must reflect cart membership, specify the comparison key as product.id (remove ambiguity about storing duplicate ids in cart items), and add a concrete policy for recently viewed items (e.g., “최근 본 상품은 최대 10개를 유지”). Update all similar question-form bullets (including the other instances mentioned) to the same declarative style so the README reads as explicit specs rather than meeting notes.app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailUiState.kt (1)
11-11: ⚡ Quick winUI 상태에 도메인 모델이 직접 노출되어 계층 분리가 약해집니다
Line 11의
latestProduct: Product?는 UI 계층이 도메인 타입에 직접 의존하게 만듭니다. 이번 PR 목표(UIView 모델 분리)에 맞추려면 화면 전용 모델로 경계를 유지하는 쪽이 더 일관적입니다.🤖 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/features/productDetail/ProductDetailUiState.kt` at line 11, ProductDetailUiState currently exposes the domain Product via the latestProduct: Product? property which breaks layering; change latestProduct to a UI-specific model (e.g., LatestProductUiModel or ProductViewState?) and map the domain Product to that UI model before populating ProductDetailUiState. Update places that construct ProductDetailUiState (search for ProductDetailUiState usage) to perform the mapping from domain Product -> the new UI model and remove direct Product references from UI layer.app/src/test/java/woowacourse/shopping/cart/CartItemTest.kt (1)
37-55: ⚡ Quick win경계 입력 테스트를 한두 케이스 더 추가하면 안정성이 크게 올라갑니다.
지금은 양수 delta 중심이라,
0/음수 delta나현재 수량보다 큰 감소 요청같은 케이스에서 기대 동작(예외/거부)을 명시해두면 도메인 규칙 회귀를 더 잘 막을 수 있습니다.🤖 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/cart/CartItemTest.kt` around lines 37 - 55, Add boundary tests around CartItem operations: add cases using cartItem1 that call increaseQuantity(0) and increaseQuantity(-1) and assert the domain's expected behavior (e.g., throw an exception such as IllegalArgumentException or leave quantity unchanged), and add a test that calls decreaseQuantity with a delta greater than the current quantity (and assert it throws the expected exception or is rejected). Use the existing helpers/fixtures (cartItem1) and the methods increaseQuantity and decreaseQuantity so these new tests live alongside the existing ones and explicitly verify invalid/edge deltas and over-decrement behavior.app/src/test/java/woowacourse/shopping/cart/CartItemsTest.kt (1)
17-29: ⚡ Quick win수량 변경 핵심 시나리오에 직접적인 검증을 추가해 주세요.
현재는 존재 여부/총합 위주라서 수량 계산 회귀를 놓치기 쉽습니다.
addCartItem/minusCartItem이후getQuantity(targetCartItem)를 함께 검증하면 의도한 동작을 더 정확히 보호할 수 있습니다.Also applies to: 69-82
🤖 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/cart/CartItemsTest.kt` around lines 17 - 29, Add an explicit quantity assertion after addCartItem/minusCartItem calls: when testing CartItems.addCartItem and CartItems.minusCartItem (in CartItemsTest), after obtaining addedCartItems/updatedCartItems assert that addedCartItems.getQuantity(targetCartItem) (or getQuantity on the returned instance) equals the expected CartItemQuantity (e.g. CartItemQuantity(targetQuantity or decremented value)) in addition to the existing searchCartItem/total checks; reference CartItems, addCartItem, minusCartItem, getQuantity, CartItemQuantity and searchCartItem to locate where to add the assertions.app/src/main/java/woowacourse/shopping/features/productList/ProductListActivity.kt (1)
65-68: ⚡ Quick win존재 여부 검증 흐름을 한 번에 판단 가능한 형태로 정리해보면 어떨까요?
지금 패턴은 “메서드 호출(부수효과) → 별도 플래그 확인”이라 타이밍/상태 동기화 이슈가 숨어들기 쉽습니다. 클릭 시점에 즉시 판단 가능한 반환값(or 단일 상태 이벤트)으로 합치면 흐름이 더 안전해집니다.
🤖 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/features/productList/ProductListActivity.kt` around lines 65 - 68, Current flow calls viewModel.isHasProductId(productUi.id) for side effects and then checks viewModel.isHasProductId flag, which risks timing/state sync issues; change the API so the check is atomic: modify the method isHasProductId(...) to return a Boolean (or provide a single single-shot state/event like hasProductIdEvent) and use that returned value directly in the click handler (e.g., val exists = viewModel.isHasProductId(productUi.id); if (!exists) { Toast... }). Update any callers and the ViewModel implementation (isHasProductId function and any backing property) to reflect the synchronous boolean-return contract or emit a single state event.app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailActivity.kt (1)
25-27: ⚡ Quick win
setContent블록 내에서의finish()호출을 Activity 초기화 단계로 분리해 보세요.현재 코드에서
parcelProduct가 null일 때setContent내부에서finish()를 호출하면, Compose 재구성 시점에 예측하기 어려운 동작이 발생할 수 있습니다.
onCreate에서 데이터 검증을 먼저 수행한 후, 필요한 경우finish()를 호출하고setContent를 진행하는 흐름으로 개선하면 어떨까요? 이렇게 하면 Activity의 생명주기와 Compose의 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/features/productDetail/ProductDetailActivity.kt` around lines 25 - 27, 현재 parcelProduct null 체크와 finish() 호출이 setContent 블록 내부에 있어 Compose 재구성 시 불안정한 동작을 유발할 수 있으니, onCreate 진입부에서 parcelProduct를 먼저 검증하고 null이면 finish() 후 즉시 반환하도록 이동하세요; 즉, 참조되는 식별자(parcelProduct, setContent, onCreate, finish())를 찾아 setContent를 호출하기 전에 null 체크를 수행하고, null일 경우 finish() 호출 후 더 이상 setContent를 실행하지 않도록 리팩터링하세요.app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailViewModel.kt (3)
125-137: 💤 Low valueViewModelProvider.Factory의 최신 권장 형태 검토(선택)
현재 구현은
create(modelClass: Class<T>): T시그니처를 오버라이드하는 전통적 방식입니다. 동작에 문제는 없지만, AndroidX Lifecycle 2.5+ 이후로는viewModelFactory { initializer<ProductDetailViewModel> { ... } }DSL을 권장합니다. 보일러플레이트와 안전하지 않은 캐스트(as T)를 줄일 수 있으니, 시간이 되시면 적용해 보시고 차이를 비교해 보세요.Lifecycle ViewModel — Factory APIs 문서를 참고하시면 됩니다.
🤖 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/features/productDetail/ProductDetailViewModel.kt` around lines 125 - 137, The current ProductDetailViewModelFactory uses the old create(modelClass: Class<T>) override and an unsafe cast; replace it with the AndroidX viewModelFactory DSL to remove boilerplate and the unchecked cast: stop implementing ProductDetailViewModelFactory as a ViewModelProvider.Factory with create(), and instead provide a factory via viewModelFactory { initializer<ProductDetailViewModel> { ProductDetailViewModel(product, cartRepository, recentProductRepository) } } (or equivalent registration) so consumers obtain the instance without using as T and without manual class checks; reference ProductDetailViewModelFactory, create, and ProductDetailViewModel when locating the code to change.
96-122: ⚡ Quick win누적 합산 방식의 가격 계산과 경계값 처리 점검
increaseCartItem/decreaseCartItem이price를± product.price.value로 누적/차감합니다. 이 접근법에는 몇 가지 위험이 있습니다.
- 누적 방식은 호출 누락이나 잘못된 상태 전이가 한 번이라도 일어나면
price가quantity * unitPrice와 어긋난 채로 표류합니다. 단가는 변하지 않으므로 매번quantity * unitPrice로 파생 계산하면 어떻게 될지 비교해 보세요.decreaseCartItem에는 하한 검사가 없습니다. UI에서minusEnabled = quantity > 1로 비활성화하더라도, ViewModel 메서드 단독으로는quantity = 0혹은 음수까지 내려갈 수 있는 구조입니다. 도메인 모델(CartItemQuantity)에 최소값 제약이 있는지, ViewModel에서도 가드를 둘지 고려해 보세요.- 상한도 마찬가지로 생각해 보세요. 사용자가 수량을 임의로 무한 증가시키는 시나리오는 어떻게 다루어야 할지요?
“표시 가격은 항상 단가의 함수”라는 불변식을 ViewModel 안에 강제하면 이 두 메서드는 단순히
quantity변경만 다루도록 줄어듭니다.🤖 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/features/productDetail/ProductDetailViewModel.kt` around lines 96 - 122, The current increaseCartItem/decreaseCartItem mutate a running price and allow quantity to go out of bounds; change both methods to derive price from quantity * unitPrice instead of incrementing/decrementing price directly, enforce a minimum quantity of 1 (and an optional maximum if your domain requires one) inside the ViewModel (or delegate to CartItemQuantity if present) before applying the change, and then update quantity, minusEnabled = quantity > 1, and _uiState with productPrice = quantity * product.price.value so the displayed price is always a pure function of quantity and unit price; ensure decreaseCartItem prevents going below the minimum and increaseCartItem optionally respects a configured max.
44-54: ⚡ Quick win
isLatestProduct/isLastProduct명명 일관성과 의미 명확화라인 44에서
isLatestProduct = (latestProduct?.id ?: product.id) == product.id로 계산하고, 라인 54에서는 UiState의isLastProduct필드로 매핑합니다. 다음을 검토해 보세요.
- “
latestProduct가 null이면isLatestProduct = true”라는 결론은 의도된 것인가요? 의미상 “최근 본 상품이 아직 없다”와 “지금 보고 있는 상품이 곧 최근 본 상품이다”는 다른 상태일 수 있습니다.- 필드 이름
isLastProduct는 영어 사용자에게는 “마지막 상품(목록 끝)”으로 읽힐 여지가 있습니다. UI 컴포넌트의 의도가 “직전에 본 상품 카드를 숨길지 여부”라면, 그에 부합하는 이름(예:hasPreviousProduct,shouldShowRecentCard등)을 고려해 보세요.- ViewModel 내부 변수명(
isLatestProduct)과 UiState 필드명(isLastProduct)이 다른 이유는 무엇인가요? 통일하면 화면 측의 매핑(isLatestProduct = uiState.isLastProduct)도 사라집니다.🤖 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/features/productDetail/ProductDetailViewModel.kt` around lines 44 - 54, The boolean logic and naming are inconsistent: you compute isLatestProduct = (latestProduct?.id ?: product.id) == product.id which treats null latestProduct as true, then map it into ProductDetailUiState's isLastProduct; fix by choosing a single clear name (e.g., hasPreviousProduct or shouldShowRecentCard) used both in the ViewModel and ProductDetailUiState, change the calculation to reflect intent (for example set hasPreviousProduct = latestProduct != null && latestProduct.id == product.id if you mean "this product is the previously viewed one", or hasPreviousProduct = latestProduct != null if you mean "there is a previous product"), and update the _uiState.update call to pass the unified field name (replace isLatestProduct/isLastProduct with the chosen identifier) so the ViewModel and UI state are consistent.app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailScreen.kt (1)
180-209: ⚡ Quick win“마지막으로 본 상품” 카드의 Modifier 순서 점검 필요
현재 체인 순서가
clip → padding(18.dp) → fillMaxWidth → height(80.dp) → border → clickable로 구성되어 있습니다. Compose에서 Modifier 순서는 시각/입력 동작에 직접적인 영향을 주는데, 다음 사항들을 한 번 검토해 보시면 좋겠습니다.
padding이 사이즈 지정(fillMaxWidth,height) 앞에 위치하면, 사이즈가 패딩으로 줄어든 영역 안에서 결정됩니다. 부모로부터 받은 영역 전체에 80dp 높이를 적용하고 싶었던 의도와 일치하나요?clip이border보다 앞에 있으면 보더 자체가 클립되지 않습니다. 둥근 모서리에 보더를 깔끔히 맞추려면 어떤 순서가 적절할까요?clickable이padding뒤에 있어 ripple/포커스 영역이 패딩 안쪽으로만 한정됩니다. 카드 외곽 전체가 클릭 가능 영역이 되도록 하고 싶다면 어떤 순서가 좋을지 함께 고려해 보세요.공식 문서의 Modifier 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/features/productDetail/ProductDetailScreen.kt` around lines 180 - 209, The Modifier chain on the Column should be reordered so visual size is determined before internal padding and the clickable/ripple and border align with the clipped shape: move clickable to the front, then apply clip and border, then size modifiers (fillMaxWidth, height) and finally padding; update the Column's Modifier (the modifier passed to Column, and the RoundedCornerShape usage) accordingly so the ripple covers the whole card, the border matches the rounded corners, and the 80.dp height applies to the card's outer size rather than being reduced by padding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 094bfcbb-c6f3-4073-b479-5cd70dffafbb
📒 Files selected for processing (56)
README.mdapp/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/woowacourse/shopping/ShoppingApplication.ktapp/src/main/java/woowacourse/shopping/data/DataProvider.ktapp/src/main/java/woowacourse/shopping/data/MockData.ktapp/src/main/java/woowacourse/shopping/data/NetworkMonitor.ktapp/src/main/java/woowacourse/shopping/data/cart/CartRepositoryMockImpl.ktapp/src/main/java/woowacourse/shopping/data/product/ProductRepositoryMockImpl.ktapp/src/main/java/woowacourse/shopping/data/source/local/CartDatabase.ktapp/src/main/java/woowacourse/shopping/data/source/local/dao/CartDao.ktapp/src/main/java/woowacourse/shopping/data/source/local/dao/RecentProductDao.ktapp/src/main/java/woowacourse/shopping/data/source/local/entity/CartEntity.ktapp/src/main/java/woowacourse/shopping/data/source/local/entity/RecentProductEntity.ktapp/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/cart/CartRepositoryMockImpl.ktapp/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/cart/CartRepositoryRoomImpl.ktapp/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/product/ProductRepositoryMockImpl.ktapp/src/main/java/woowacourse/shopping/data/source/local/repositoryImpl/recentProduct/RecentProductRepositoryRoomImpl.ktapp/src/main/java/woowacourse/shopping/data/source/remote/ProductEntity.ktapp/src/main/java/woowacourse/shopping/data/source/remote/ProductJsonData.ktapp/src/main/java/woowacourse/shopping/data/source/remote/ProductRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/source/remote/ProductRepositoryRemoteImpl.ktapp/src/main/java/woowacourse/shopping/data/source/remote/ShoppingMockServer.ktapp/src/main/java/woowacourse/shopping/domain/RecentProductRepository.ktapp/src/main/java/woowacourse/shopping/domain/cart/model/Cart.ktapp/src/main/java/woowacourse/shopping/domain/cart/model/CartItem.ktapp/src/main/java/woowacourse/shopping/domain/cart/model/CartItemQuantity.ktapp/src/main/java/woowacourse/shopping/domain/cart/model/CartItems.ktapp/src/main/java/woowacourse/shopping/domain/cart/repository/CartRepository.ktapp/src/main/java/woowacourse/shopping/domain/product/repository/ProductRepository.ktapp/src/main/java/woowacourse/shopping/features/cart/CartActivity.ktapp/src/main/java/woowacourse/shopping/features/cart/CartScreen.ktapp/src/main/java/woowacourse/shopping/features/cart/CartStateHolder.ktapp/src/main/java/woowacourse/shopping/features/cart/CartUiModel.ktapp/src/main/java/woowacourse/shopping/features/cart/CartUiState.ktapp/src/main/java/woowacourse/shopping/features/cart/CartViewModel.ktapp/src/main/java/woowacourse/shopping/features/generalComponent/QuantityControlRow.ktapp/src/main/java/woowacourse/shopping/features/productDetail/ParcelProduct.ktapp/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailActivity.ktapp/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailScreen.ktapp/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailStateHolder.ktapp/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailUiState.ktapp/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailViewModel.ktapp/src/main/java/woowacourse/shopping/features/productList/ProductListActivity.ktapp/src/main/java/woowacourse/shopping/features/productList/ProductListScreen.ktapp/src/main/java/woowacourse/shopping/features/productList/ProductListStateHolder.ktapp/src/main/java/woowacourse/shopping/features/productList/ProductListUiState.ktapp/src/main/java/woowacourse/shopping/features/productList/ProductListViewModel.ktapp/src/main/java/woowacourse/shopping/features/productList/ProductUiModel.ktapp/src/test/java/woowacourse/shopping/cart/CartItemQuantityTest.ktapp/src/test/java/woowacourse/shopping/cart/CartItemTest.ktapp/src/test/java/woowacourse/shopping/cart/CartItemsTest.ktapp/src/test/java/woowacourse/shopping/cart/CartTest.ktapp/src/test/java/woowacourse/shopping/data/cart/CartRepositoryMockImplTest.ktapp/src/test/java/woowacourse/shopping/data/product/ProductRepositoryMockImplTest.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (5)
- app/src/main/java/woowacourse/shopping/features/productDetail/ProductDetailStateHolder.kt
- app/src/main/java/woowacourse/shopping/data/cart/CartRepositoryMockImpl.kt
- app/src/main/java/woowacourse/shopping/features/cart/CartStateHolder.kt
- app/src/main/java/woowacourse/shopping/data/product/ProductRepositoryMockImpl.kt
- app/src/main/java/woowacourse/shopping/features/productList/ProductListStateHolder.kt
| alias(libs.plugins.kotlin.android) | ||
| alias(libs.plugins.kotlin.compose) | ||
| alias(libs.plugins.ksp) | ||
| kotlin("plugin.serialization") version "2.1.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kotlin Gradle plugin 2.3.0과 org.jetbrains.kotlin.plugin.serialization 2.1.0을 함께 사용할 수 있나요? 공식 호환성 문서 링크와 권장 버전 매칭 규칙을 알려주세요.
💡 Result:
아니요, Kotlin Gradle plugin 2.3.0과 org.jetbrains.kotlin.plugin.serialization 2.1.0을 함께 사용할 수 없습니다. serialization 플러그인은 Kotlin 컴파일러 버전(즉, Kotlin Gradle plugin 버전)과 정확히 일치해야 합니다.[1][2][3][4] 공식 문서와 예시에서 항상 동일한 버전을 사용하도록 명시되어 있습니다: - https://kotlinlang.org/docs/serialization.html (공식 Kotlin 문서, plugins { kotlin("jvm") version "2.3.21" kotlin("plugin.serialization") version "2.3.21" } [2]) - https://github.com/Kotlin/kotlinx.serialization (공식 GitHub README, kotlin("plugin.serialization") version "2.3.0" with kotlin("jvm") version "2.3.0" [1][3]) 호환성 정책: serialization 플러그인은 Kotlin 버전과 tandem으로 릴리스되며, 버전 불일치 시 컴파일 오류나 런타임 문제가 발생합니다. 예를 들어, 과거 이슈에서 Kotlin 1.9.10과 serialization 1.9.20 불일치가 문제를 일으켰습니다.[4][5] 권장 버전 매칭 규칙: Kotlin Gradle plugin (kotlin("jvm") 등) 버전과 org.jetbrains.kotlin.plugin.serialization 버전을 항상 동일하게 맞추세요. 예: plugins { kotlin("jvm") version "2.3.0" kotlin("plugin.serialization") version "2.3.0" }[1][2][6]
Citations:
- 1: https://github.com/kotlin/kotlinx.serialization
- 2: https://kotlinlang.org/docs/serialization.html
- 3: https://github.com/Kotlin/kotlinx.serialization
- 4: Add build-time check for Kotlin compiler version and kotlinx.serialization plugin version compatibility Kotlin/kotlinx.serialization#2393
- 5: Android, Gradle, Serialization plugin is incompatible with Kotlin Android plugin 1.9.20 Kotlin/kotlinx.serialization#2508
- 6: https://github.com/Kotlin/kotlinx.serialization/blob/master/gradle/libs.versions.toml
Kotlin Serialization 플러그인 버전을 Kotlin 플러그인과 맞춰야 합니다.
Line 6의 kotlin("plugin.serialization") version "2.1.0"을 프로젝트의 Kotlin 버전 2.3.0으로 통일하세요. Kotlin Serialization 플러그인은 Kotlin 컴파일러 버전과 정확히 일치해야 하며, 공식 Kotlin 문서(kotlinlang.org/docs/serialization.html)에서도 동일 버전 사용을 명시하고 있습니다. 버전 불일치 시 컴파일 오류가 발생합니다.
🤖 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 6, The kotlin serialization plugin version is
mismatched; update the declaration kotlin("plugin.serialization") version
"2.1.0" to use the project's Kotlin version (2.3.0) so the serialization plugin
matches the Kotlin compiler version; locate the kotlin("plugin.serialization")
line in build.gradle.kts and change its version string to "2.3.0".
| @Query("SELECT * FROM cart_table") | ||
| suspend fun getAll(): List<CartEntity> | ||
|
|
||
| @Insert(onConflict = OnConflictStrategy.Companion.REPLACE) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android Room의 @insert(onConflict=...)에서 Kotlin 표기는 OnConflictStrategy.REPLACE가 맞나요? OnConflictStrategy.Companion.REPLACE 사용 가능 여부를 공식 문서 기준으로 확인해줘.
💡 Result:
결론: Room의 @Insert(onConflict=...)에서 Kotlin 표기는 OnConflictStrategy.REPLACE가 맞습니다. 공식 문서에서도 @Insert(onConflict = OnConflictStrategy.REPLACE)처럼 사용 예시와 REPLACE 상수가 명시돼 있습니다. [1][2] OnConflictStrategy.Companion.REPLACE 사용 가능 여부: 공식 문서(= API reference)에서 OnConflictStrategy는 “REPLACE 상수”로 제공되며, 사용 예시는 OnConflictStrategy.REPLACE 형태입니다. API reference에는 Companion.REPLACE 같은 형태를 사용하도록 안내/명시되어 있지 않습니다. 따라서 공식 문서 기준으로는 OnConflictStrategy.Companion.REPLACE를 써야 한다고 보기는 어렵고, 표준/권장 사용은 OnConflictStrategy.REPLACE입니다. [3][4][1] 참고로, 공식 문서에서 OnConflictStrategy.REPLACE의 의미는 “기존 행을 대체하고 트랜잭션을 계속”하는 전략입니다. [3][1]
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/room/Insert
- 2: https://developer.android.com/training/data-storage/room/accessing-data
- 3: https://developer.android.com/reference/kotlin/androidx/room/OnConflictStrategy
- 4: https://developer.android.com/reference/androidx/room/OnConflictStrategy
OnConflictStrategy.Companion.REPLACE 표기는 Room 공식 API 문서에 없습니다.
Android 공식 문서(androidx.room.OnConflictStrategy API reference)를 보면, @Insert(onConflict = OnConflictStrategy.REPLACE) 형태가 표준입니다. 현재 Companion을 거쳐서 접근하는 방식은 공식 권장과 맞지 않으므로 컴파일 오류가 발생할 가능성이 높습니다.
Line 15에서 Kotlin의 표준 상수 접근 방식으로 어떻게 수정할 수 있을지 한번 생각해보셨나요?
🤖 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/source/local/dao/CartDao.kt` at
line 15, Replace the non-standard usage of the Room conflict constant by
changing the `@Insert` annotation to use the enum constant directly: replace
OnConflictStrategy.Companion.REPLACE with OnConflictStrategy.REPLACE in the
CartDao (the `@Insert` annotation on the DAO method) so it follows the Room API
and Kotlin constant access conventions.
| init { | ||
| viewModelScope.launch { | ||
| addRecentProducts(product.id) | ||
|
|
||
| name = product.name.value | ||
| imageUrl = product.imageUrl.value | ||
| price = product.price.value | ||
| quantity = 1 | ||
| minusEnabled = false | ||
| latestProduct = recentProductRepository.getMostRecentProduct() | ||
| isLatestProduct = (latestProduct?.id ?: product.id) == product.id | ||
|
|
||
| _uiState.update { | ||
| ProductDetailUiState( | ||
| productName = name, | ||
| productImageUrl = imageUrl, | ||
| productPrice = price, | ||
| quantity = quantity, | ||
| minusEnabled = minusEnabled, | ||
| latestProduct = latestProduct, | ||
| isLastProduct = isLatestProduct, | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
init에서 최근 본 상품 기록과 조회 사이의 경합 조건 가능성
addRecentProducts(product.id)는 내부에서 viewModelScope.launch { ... }로 별도 코루틴을 띄우기 때문에, 이 함수는 await하지 않고 즉시 반환됩니다. 그 직후 recentProductRepository.getMostRecentProduct()를 호출하면 다음 두 시나리오가 비결정적으로 발생합니다.
- write가 read보다 먼저 완료된 경우: 가장 최근 상품 = 방금 본 상품 ⇒
isLatestProduct = true, 카드 숨김 - write가 read보다 늦게 완료된 경우: 가장 최근 상품 = 직전에 본 다른 상품 ⇒ 카드 표시
기능 명세상 “마지막으로 본 상품” 카드는 어느 쪽이 정답인가요? 또한 다음을 확인해 보세요.
addRecentProduct는 suspend 함수인가요? 그렇다면 굳이 내부에서launch로 감싸야 하는 이유는 무엇인가요? 호출 측 코루틴에서await하도록 두는 편이 순서 보장에 유리하지 않을까요?- 만약 fire-and-forget이 필요한 자리가 따로 있다면,
init처럼 “기록 → 조회”의 순서가 중요한 자리에서는 다른 동기화 방법을 쓸 수 있는지 검토해 보세요.
🤖 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/features/productDetail/ProductDetailViewModel.kt`
around lines 34 - 58, The init block currently races because
addRecentProducts(product.id) launches a separate coroutine and returns
immediately before recentProductRepository.getMostRecentProduct() runs; change
the flow so the write completes before the read: make addRecentProducts a
suspend function (or expose a suspend helper) and call it directly from the init
coroutine (instead of having addRecentProducts launch its own coroutine), or
have addRecentProducts return a Job and call .join() before calling
recentProductRepository.getMostRecentProduct(); update
ProductDetailViewModel.init to await the write, then read latestProduct and
compute isLatestProduct, and then update _uiState so the "last viewed product"
decision is deterministic.
There was a problem hiding this comment.
코드래빗의 코멘트를 잘 읽어보시고 현재 어떤 문제가 왜 발생하는지 확인해보시면 좋겠네요.
| fun addToCart() { | ||
| viewModelScope.launch { | ||
| val cartItem = CartItem(product = product, quantity = CartItemQuantity(1)) | ||
| cartRepository.addCartItem(cartItem, quantity) | ||
| } | ||
| } |
There was a problem hiding this comment.
addToCart가 사용자가 선택한 수량을 무시할 가능성
val cartItem = CartItem(product = product, quantity = CartItemQuantity(1))
cartRepository.addCartItem(cartItem, quantity)CartItem에 들어가는 수량은 1로 하드코딩되어 있고, 별도로 quantity가 두 번째 인자로 전달되고 있습니다. 다음을 확인해 보세요.
CartRepository.addCartItem(cartItem, quantity)의 시그니처에서 두 번째quantity인자가 무엇을 의미하나요?cartItem.quantity와의 관계는 어떻게 정의되어 있나요?- 사용자가 상세 화면에서 수량을 3으로 올린 뒤 “장바구니 담기”를 눌렀을 때, 실제로 장바구니에 3이 반영되는지 동작/테스트로 확인되어 있나요?
- 만약 두 번째 인자가 “증가량”을 의미한다면,
CartItem의 수량을 1로 고정하는 의도는 무엇인가요?
명확한 단일 API(예: addCartItem(CartItem(product, CartItemQuantity(quantity))))로 모으는 편이 호출 측 실수와 의미 충돌을 줄여줍니다.
🤖 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/features/productDetail/ProductDetailViewModel.kt`
around lines 89 - 94, The addToCart function builds CartItem with a hardcoded
CartItemQuantity(1) while also passing a separate quantity to
cartRepository.addCartItem, which risks ignoring the user's selected quantity;
change addToCart so the CartItem is constructed using the selected quantity (use
CartItem(product = product, quantity = CartItemQuantity(quantity))) and then
call cartRepository.addCartItem with a single, unambiguous API (either pass the
CartItem only or ensure the second parameter semantics match and remove the
hardcoded 1) to eliminate the discrepancy between CartItem.quantity and the
repository call.
There was a problem hiding this comment.
현재 CartItem이 수량을 나타내는 Quantity프로퍼티를 갖고 있지만, 실제로 DB에 값을 추가하는 로직은 어떻게 되나요?
id를 기반으로 저장되어 있는 수량을 가져온 뒤에 targetQuantity를 더해서 다시 저장을 합니다.
Quantity를 가지고 있는 의미가 있는 것일까요?
There was a problem hiding this comment.
이 리뷰에 대한 답변입니다.
이 어노테이션에서 나타내는 경고는 Parcelize 어노테이션의 패키지가 변경되었다는 경고입니다. 그렇기에 kotlinx.android.parcel에서 kotlinx.parcelize로 import 경로를 변경하였습니다.
| val product = intent.getParcelableExtra<ParcelProduct>("PRODUCT")!! | ||
| val stateHolder = | ||
| ProductDetailStateHolder( | ||
| cartRepository = DataProvider.cartRepository, | ||
| ) | ||
| val parcelProduct = intent.getParcelableExtra<ParcelProduct>("PRODUCT") |
There was a problem hiding this comment.
이 리뷰에 대한 답변입니다!
Product가 넘어왔을 때 잘못되는 상황에 대해서 찾아봤습니다.
- key가 불일치할 때
- 타입이 불일치할 때
- Intent에 아무것도 넣지 않고 보냈을 때
이런 상황들이 언박싱했을 때 null을 반환하는 상황이였습니다. 즉, 대개 잘못된 값을 언박싱했을 때는 null이 반환되는 사실을 알았습니다.
그러면 크롱이 남겨주신 리뷰의 의도를 생각해봤을 때, !!로 강제로 언박싱했을 때 잘못된 값을 풀면 생기는 문제를 해결하면 될 것입니다.
잘못된 값을 언박싱했을 때 null이 나오게되니, parcelProduct == null일 경우를 처리하면 된다고 생각했습니다. 그리고 null일 경우에는 finish()를 사용해 엑티비티를 끝내서, 상세 화면으로 넘어가지 못하게 막을 수 있습니다.
There was a problem hiding this comment.
그리고 현재 잘 보면 컴파일러가 무언가를 알려주고 있을거에요.
어떻게 대응해볼 수 있을지 고민해보셔도 좋겠습니다.
| items = pageCartItems, | ||
| key = { item -> item.id }, |
There was a problem hiding this comment.
이 리뷰에 대한 답변입니다.
compose가 id기반으로 리컴포지션을 감지하기 때문에 key를 적용해놓으면 변경된 아이템만 추적해서 불필요한 리컴포지션을 줄일 수 있습니다!
그래서 LazyColumn에 상품의 id를 이용해 고유한 값을 key로 적용했습니다.
| private val _uiState = MutableStateFlow(ProductDetailUiState()) | ||
| val uiState: StateFlow<ProductDetailUiState> = _uiState.asStateFlow() | ||
|
|
||
| private var product: Product = parcelProduct.toProduct() | ||
| private var name = "" | ||
| private var imageUrl = "" | ||
| private var price = 0 | ||
| private var quantity = 0 | ||
| private var minusEnabled = false | ||
| private var isLatestProduct = true | ||
| private var latestProduct: Product? = null |
There was a problem hiding this comment.
두 부분이 비슷하다고 느껴져 한 코멘트로 답변드리겠습니다!
이 부분도 가시성을 private로 바꿔야하죠.
하지만 이렇게 상태를 각각 관리한다면 각각 변수를 관리해야되기에 유지보수에도 힘들고, 놓칠 수도 있다는 문제점이 있다는 것을 알았습니다. uiState라는 상태를 묶어서 UI에 보여지는 상태를 관리하는 방법이 있다는 것을 배웠습니다.
uiState로 묶는다면 viewModel에서 관리할 때는 아래와 같은 하나의 상태 묶음만 mutableStateOf로 추적하고, ui에 그릴 때도 uiState에서 관리되는 상태들을 호출하여 그릴 수 있습니다.
또한 ui에만 그려지는 도메인 상태들을 묶은 uiModel이라는 것을 만들었습니다. 이 uiModel이라는 것을 만든 이유는 uiModel이 없었을 경우 도메인 객체를 그대로 ui에 가져와서 그리게 된다는 점이 있었습니다. 이를 해결하려면 ui와 domain의 연결을 끊어야하기에 중간에 무언가가 필요했습니다.
그래서 ui 그리기에 필요한 도메인 상태들만을 묶은 uiModel이라는 것을 만들게 되었습니다. 이런 방식이면 viewModel에서만 도메인과 ui의 상호작용이 이루어지고, ui와 domain 사이에는 간섭이 없기에 의존성이 분리됩니다!
| fun ProductUiModel.toProduct(): Product = | ||
| Product( | ||
| id = id, | ||
| name = ProductName(name), | ||
| price = Price(price), | ||
| imageUrl = ImageUrl(imageUrl), | ||
| ) |
There was a problem hiding this comment.
이 리뷰에 대한 답변입니다.
이 리뷰를 받고 사실 이해가 잘 되지 않았었습니다. 그래서 몇몇 크루들은 직렬화를 어떻게 했고, 어디에 구현했는지를 살짝, 아주 살짝 참고해봤습니다.
대체로 크롱의 말처럼 변환 함수가 그 클래스와 같은 위치에 존재했습니다. 위치보다 더 큰 깨달음을 얻은 것은 확장함수 개념을 사용한 것이였습니다.
저는 싱글톤을 사용해 전역에서 변환 함수를 불러왔었습니다. 이런 방식은 싱글톤이 선언된 클래스를 앞에 붙여야하며, 함수의 인자로 그 객체를 넣어줘야 했습니다. 하지만 확장 함수를 선언한다면, 그 객체 뒤에 확장함수로 선언한 함수를 붙여주면 쉽게 변환할 수 있었습니다.
그래서 해당 클래스로 변환하는 곳에 확장함수를 선언하면서 싱글톤을 지향하고 더 깔끔하게 변환을 할 수 있는 방법으로 수정하였습니다!
|
|
||
| fun loadCartPage() { | ||
| viewModelScope.launch { | ||
| if (!Cart.isPageValid(currentPage)) currentPage = 0 |
There was a problem hiding this comment.
이 리뷰에 대한 답변입니다!
하나를 고쳤다 생각하면 다른 곳에서 또 구멍이 나고…. 리뷰를 받을 때마다 계속 무언가를 놓치게 되는 것 같네요…
확실히 UI에서 원천적으로 들어오는 것은 막았지만, 도메인 자체에서는 getPage()에 들어가는 인자를 검증할 수 없기에 getPage(-1, 0)같은 식으로 도메인을 따로 호출한다면 예외가 발생해서 앱이 터질 것입니다.
그래서 기존 아이디어와 이번에 반영한 아이디어를 섞어서 해결해보고자 했습니다.
도메인에서 page의 값을 검증하는 isPageValid()라는 함수를 싱글톤으로 만들어 getPage()에서 require을 사용해 page 값이 조건과 맞지 않는다면 예외를 던집니다. 이것만 사용했을 때의 문제점이 UI에서 getPage()를 호출하며 page가 조건과 일치하지 않는다면 앱이 터진다는 것이였습니다.
이 문제를 해결하기 위해 싱글톤으로 도메인과 같은 조건을 UI에서 먼저 currentPage가 isPageValid를 만족하는지를 검증해보고, 조건과 일치하지 않는다면 현재 페이지를 0으로 만드는 방식으로 문제를 해결했습니다.
즉, 도메인과 UI에서 같은 page 검증 조건을 사용하는 것이죠. 그렇게 한다면 도메인 스스로도 검증할 수 있으면서 UI에서 도메인과 동일한 검증 조건으로 먼저 검증하고 처리하기 때문에 앱이 터질 일도 없다고 생각했습니다.
There was a problem hiding this comment.
해당 리뷰를 반영하면서 도메인이 왜 자신만의 규칙을 가지고 있어야 하는지, UI에서 방어로직을 만들면 되는 것이 아닌가 하는 부분에 대해 좀 더 명확히 이해하신 것 같아 좋습니다.
하나를 고쳤다 생각하면 다른 곳에서 또 구멍이 나고…. 리뷰를 받을 때마다 계속 무언가를 놓치게 되는 것 같네요…
지금 배워가는 단계이고, 이러한 이유 때문에 코드리뷰가 필요한 것이 아닐까요? 😄
krrong
left a comment
krrong
left a comment
There was a problem hiding this comment.
안녕하세요 하로,
이번 미션 구현하느라 고생 많으셨습니다.
전반적으로 잘 구현해주신 것 같은데요, 이전 미션에서 받은 코멘트들도 잊지 않고 잘 반영해주셨네요 👍
이전 미션에서 되돌렸던 것들을 이번 미션에서 다시 적용해보셨을텐데, 이번에는 어느정도의 이해를 기반으로 구현해보는 경험이었으면 좋겠습니다.
고민해볼만한 부분들에 코멘트를 남겨두었으니, 코드래빗의 코멘트와 함께 확인해주세요 🙇
| init { | ||
| viewModelScope.launch { | ||
| addRecentProducts(product.id) | ||
|
|
||
| name = product.name.value | ||
| imageUrl = product.imageUrl.value | ||
| price = product.price.value | ||
| quantity = 1 | ||
| minusEnabled = false | ||
| latestProduct = recentProductRepository.getMostRecentProduct() | ||
| isLatestProduct = (latestProduct?.id ?: product.id) == product.id | ||
|
|
||
| _uiState.update { | ||
| ProductDetailUiState( | ||
| productName = name, | ||
| productImageUrl = imageUrl, | ||
| productPrice = price, | ||
| quantity = quantity, | ||
| minusEnabled = minusEnabled, | ||
| latestProduct = latestProduct, | ||
| isLastProduct = isLatestProduct, | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
코드래빗의 코멘트를 잘 읽어보시고 현재 어떤 문제가 왜 발생하는지 확인해보시면 좋겠네요.
| fun addToCart() { | ||
| viewModelScope.launch { | ||
| val cartItem = CartItem(product = product, quantity = CartItemQuantity(1)) | ||
| cartRepository.addCartItem(cartItem, quantity) | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 CartItem이 수량을 나타내는 Quantity프로퍼티를 갖고 있지만, 실제로 DB에 값을 추가하는 로직은 어떻게 되나요?
id를 기반으로 저장되어 있는 수량을 가져온 뒤에 targetQuantity를 더해서 다시 저장을 합니다.
Quantity를 가지고 있는 의미가 있는 것일까요?
| private val _uiState = MutableStateFlow(ProductDetailUiState()) | ||
| val uiState: StateFlow<ProductDetailUiState> = _uiState.asStateFlow() | ||
|
|
||
| private var product: Product = parcelProduct.toProduct() | ||
| private var name = "" | ||
| private var imageUrl = "" | ||
| private var price = 0 | ||
| private var quantity = 0 | ||
| private var minusEnabled = false | ||
| private var isLatestProduct = true | ||
| private var latestProduct: Product? = null |
|
|
||
| fun loadCartPage() { | ||
| viewModelScope.launch { | ||
| if (!Cart.isPageValid(currentPage)) currentPage = 0 |
There was a problem hiding this comment.
해당 리뷰를 반영하면서 도메인이 왜 자신만의 규칙을 가지고 있어야 하는지, UI에서 방어로직을 만들면 되는 것이 아닌가 하는 부분에 대해 좀 더 명확히 이해하신 것 같아 좋습니다.
하나를 고쳤다 생각하면 다른 곳에서 또 구멍이 나고…. 리뷰를 받을 때마다 계속 무언가를 놓치게 되는 것 같네요…
지금 배워가는 단계이고, 이러한 이유 때문에 코드리뷰가 필요한 것이 아닐까요? 😄
| ProductUiModel( | ||
| id = product.id, | ||
| name = product.name.value, | ||
| price = product.price.value, | ||
| imageUrl = product.imageUrl.value, | ||
| quantity = | ||
| cartRepository.getQuantity( | ||
| CartItem( | ||
| product = product, | ||
| quantity = CartItemQuantity(1), | ||
| ), | ||
| ), | ||
| isExistProductToCart = | ||
| cartRepository.isCartItemExist( | ||
| CartItem( | ||
| product = product, | ||
| quantity = CartItemQuantity(1), | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
ProductUiModel.toProduct 확장함수를 만들어주신 것처럼 Product.toUiModel이라는 확장함수를 만들어줄 수도 있습니다.
그리고 이러한 역할을 하는 친구들을 일반적으로는 Mapper라고 부르기도 해요 😄
| import kotlinx.coroutines.flow.callbackFlow | ||
| import kotlinx.coroutines.flow.distinctUntilChanged | ||
|
|
||
| class NetworkMonitor( |
There was a problem hiding this comment.
NetworkMonitor는 무엇을 하는 녀석인지 설명해주시고, 현재 단계에서 필요한 녀석인지 한 번 고민해볼까요?
| cartItem: CartItem, | ||
| targetQuantity: Int, | ||
| ) { | ||
| var currentQuantity = getQuantity(cartItem) |
| val product = intent.getParcelableExtra<ParcelProduct>("PRODUCT")!! | ||
| val stateHolder = | ||
| ProductDetailStateHolder( | ||
| cartRepository = DataProvider.cartRepository, | ||
| ) | ||
| val parcelProduct = intent.getParcelableExtra<ParcelProduct>("PRODUCT") |
There was a problem hiding this comment.
그리고 현재 잘 보면 컴파일러가 무언가를 알려주고 있을거에요.
어떻게 대응해볼 수 있을지 고민해보셔도 좋겠습니다.
| abstract fun recentProductDao(): RecentProductDao | ||
|
|
||
| companion object { | ||
| @Volatile |
There was a problem hiding this comment.
@Volatile 어노테이션은 어떤 역할을 하며, 무엇을 위해 추가해주셨을까요?
| class ProductRepositoryRemoteImpl( | ||
| private val remoteDataSource: ProductRemoteDataSource, |
There was a problem hiding this comment.
ProductRepositoryRemoteImpl는 단순히 ProductRemoteDataSource를 생성자로 주입받고 있습니다.
그리고 항상 API를 통해 외부의 값을 받아오는 역할을 받아오기만 해요.
현재 시점에서 Repository와 DataSource가 분리되어 얻는 이점은 무엇이라고 생각하는지 하로의 의견이 궁금합니다.
2 participants
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?기능 구현 목록
스크린샷
상품 목록 화면
상품 상세 화면
장바구니 화면
네트워크 끊겼을 시
영상
KakaoTalk_Video_2026-05-11-10-42-27.mp4
2.mp4
리뷰어에게
안녕하세요 크롱! 하로입니다. 🐲
배운 내용을 이해하며 적용해보다보니 살짝 늦어진 것 같습니다... 😅
또한 이전 리뷰도 반영하여 코멘트를 달아놨습니다!
이번 리뷰도 잘 부탁드립니다~~!!