[3단계 - 상태 관리 | 4단계 - HTTP Client] 사무엘 미션 제출합니다. #182
Open
Redish03 wants to merge 33 commits into
Open
Conversation
- 만약 없는 아이템이라면 생성 후 추가 - 있다면 개수만큼만 추가
- Create `RecentlyViewedSection` and `RecentItem` components to display a horizontal list of recently viewed products. - Integrate `RecentlyViewedSection` into `CatalogScreen` body with a spacer divider. - Add `LastViewedProductBox` to `ProductDetailScreen` to highlight the most recently viewed item. - Update `CatalogViewModel` to expose `recentProducts` state. - Update preview image URIs for `ShoppingItem` and `CartItem`.
작업 개요이 PR은 쇼핑카트 애플리케이션의 상태 관리 아키텍처를 근본적으로 재구성합니다. 기존의 액티비티 결과 기반 장바구니 전달 방식에서 Room 데이터베이스와 저장소 패턴 기반의 중앙 집중식 상태 관리로 전환합니다. Walkthrough이 변경사항은 도메인 모델을 개선하여 예상 코드 리뷰 노력🎯 4 (복잡) | ⏱️ ~45분 근거: 도메인 모델 전면 재구성, Room 데이터베이스 신규 도입, 저장소 패턴 구현, ViewModel 추가, 여러 UI 컴포넌트 신규 추가 및 리팩토링으로 인한 높은 복잡도. 다양한 파일과 구조적 변경으로 인해 각 계층을 별도로 검토해야 하는 이질성이 높습니다. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (23)
app/src/main/java/woowacourse/shopping/data/entity/RecentProductEntity.kt (1)
10-10: ⚡ Quick win기본값 설계에 대해 고민해보면 좋을 것 같습니다.
viewedAt의 기본값으로System.currentTimeMillis()를 사용하는 것은 동작하지만, 몇 가지 고려사항이 있습니다:
- 테스트 작성 시 시간을 제어하기 어려워질 수 있습니다
- 엔티티 생성 시점의 의도가 명확하지 않을 수 있습니다
다음 질문들을 생각해보세요:
- 타임스탬프를 생성하는 책임이 엔티티에 있는 것이 적절한가요?
- DAO나 Repository 레벨에서 명시적으로 타임스탬프를 제공하는 것이 더 명확하지 않을까요?
- 테스트에서 과거 시간의 엔티티를 생성해야 한다면 어떻게 할 수 있을까요?
관련 문서: 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/entity/RecentProductEntity.kt` at line 10, The RecentProductEntity currently sets viewedAt: Long = System.currentTimeMillis() inside the entity; remove the implicit time generation from RecentProductEntity (viewedAt) and require callers to supply the timestamp instead (or add a factory/constructor overload that accepts a Clock/Instant provider), then set timestamps at the creation point in the repository/DAO (or test helper) so tests can inject deterministic times and production code can use System.currentTimeMillis() there; update usages that construct RecentProductEntity to pass an explicit viewedAt (or to use the new factory with an injectable clock).app/src/main/java/woowacourse/shopping/ui/component/item/AddCircleButton.kt (2)
31-35: ⚡ Quick win접근성에 대해 생각해볼 필요가 있습니다.
Icon의
contentDescription이null로 설정되어 있습니다.다음 사항들을 고려해보세요:
- 스크린 리더를 사용하는 사용자가 이 버튼의 목적을 어떻게 알 수 있을까요?
- "장바구니에 추가" 같은 의미있는 설명을 제공하면 어떨까요?
- Android의 접근성 가이드라인에서는 무엇을 권장하고 있나요?
관련 문서: Jetpack Compose 접근성 가이드
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/component/item/AddCircleButton.kt` around lines 31 - 35, The Icon currently sets contentDescription = null which makes the button inaccessible; update the AddCircleButton composable (in AddCircleButton.kt) to provide a meaningful contentDescription (e.g., use stringResource(R.string.add_to_cart) or accept a contentDescription parameter) instead of null, and wire that string resource (localized) into the Icon(painter = painterResource(R.drawable.ic_add), contentDescription = /* meaningful string */) so screen readers will announce the button purpose.
22-29: ⚡ Quick win더 나은 Compose 패턴을 고려해보세요.
Surface에clickablemodifier를 직접 적용하는 현재 구현도 동작하지만, 개선할 여지가 있습니다.다음을 생각해보세요:
- Material3의
IconButton이나FloatingActionButton같은 컴포넌트를 사용하면 어떤 이점이 있을까요?- 리플 효과, 포커스 표시, 접근성 지원 등이 기본 제공되는 컴포넌트가 있나요?
- 현재 구조에서 시각적 피드백(예: 클릭 시 리플)이 제공되나요?
Compose Material3 문서에서 버튼 컴포넌트들을 살펴보시면 도움이 될 것 같습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/component/item/AddCircleButton.kt` around lines 22 - 29, Replace the clickable Surface in AddCircleButton.kt with a proper Material3 button component (e.g., IconButton or FloatingActionButton) instead of applying .clickable to Surface: keep the onAdd callback as the button's onClick, move size/shape/color/shadowElevation into the chosen component's parameters or its Modifier, and remove the manual .clickable modifier on Surface; this ensures built-in ripple, focus, and accessibility behavior from IconButton/FloatingActionButton and preserves the current visuals while improving affordance and accessibility.app/src/test/java/woowacourse/shopping/domain/CartProductTest.kt (1)
11-11: 💤 Low value제네릭 타입 표기 시 공백을 확인해주세요.
assertThrows <IllegalArgumentException>에서<앞의 공백은 일반적인 Kotlin 스타일과 다릅니다.assertThrows<IllegalArgumentException>로 붙여 쓰는 것이 관례입니다.🤖 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/CartProductTest.kt` at line 11, The test uses an incorrect spacing with the generic on assertThrows: change the usage in CartProductTest.kt from "assertThrows <IllegalArgumentException>" to the conventional "assertThrows<IllegalArgumentException>" (attach the generic directly to the assertThrows call) so the generic type parameter is written without a space.app/src/main/java/woowacourse/shopping/repository/productRepository/ProductRepository.kt (1)
7-8: ⚡ Quick win동기/비동기 메서드 혼용에 대해 고민해보셨나요?
getProducts는suspend함수로 비동기 처리되지만,getProductById는 일반 함수로 동기 처리됩니다.이러한 설계를 선택한 특별한 이유가 있으신가요? 두 메서드의 데이터 소스나 사용 맥락을 고려했을 때, 일관성 있는 접근 방식이 더 나을 수도 있습니다.
고려해볼 질문들:
getProductById도 Room이나 HTTP에서 데이터를 가져온다면, 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/repository/productRepository/ProductRepository.kt` around lines 7 - 8, The interface mixes async and sync APIs—getProducts is suspend while getProductById is not—so make the behavior consistent: change getProductById to a suspend function (update its signature in ProductRepository.kt to suspend fun getProductById(id: UUID): Product?) and then update all implementations and callers of ProductRepository.getProductById to use coroutine contexts (or wrap blocking calls with withContext(Dispatchers.IO) inside implementations) so database/HTTP access remains non-blocking and consistent with getProducts.app/src/main/java/woowacourse/shopping/NetworkMonitor.kt (1)
26-28: 💤 Low valueNull 처리를 더 명확하게 표현해볼까요?
Line 28의
== true패턴은 동작하지만, 의도를 더 명확하게 드러낼 수 있습니다.생각해볼 점:
capabilities?.hasCapability(...) == true가 null과 false를 모두 처리한다는 것이 명확하게 보이나요?capabilities != null && capabilities.hasCapability(...)같은 방식이 의도를 더 분명히 전달하지 않을까요?코드 리뷰어나 미래의 자신이 읽을 때 어떤 방식이 더 이해하기 쉬울지 고민해보세요.
🤖 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/NetworkMonitor.kt` around lines 26 - 28, Replace the nullable boolean expression using `== true` with an explicit null check to clarify intent: where `activeNetwork`, `capabilities`, and the `trySend(...)` call are used, change the condition from `capabilities?.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) == true` to an explicit form such as `capabilities != null && capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)` so the `trySend(...)` clearly receives a non-null boolean result.app/src/main/java/woowacourse/shopping/MockCatalog.kt (1)
11-308: ⚖️ Poor tradeoff목 데이터의 중복에 대해 생각해보셨나요?
카탈로그 리스트에 동일한 상품들이 여러 번 반복되고 있습니다 (예: "딸기주스", "무엘사" 등).
다음을 고민해보세요:
- 이렇게 중복된 데이터를 사용하는 이유는 무엇인가요?
- 페이징 테스트를 위한 것이라면, 다른 접근 방법은 없을까요?
- 실제 데이터와 목 데이터의 구조가 크게 다르면 어떤 문제가 생길 수 있을까요?
참고: 현재 구현이 테스트 목적으로 의도된 것이라면 괜찮지만, 코드 리뷰어나 미래의 개발자가 이해할 수 있도록 주석을 추가하는 것도 좋은 방법입니다.
🤖 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/MockCatalog.kt` around lines 11 - 308, The catalog list (variable catalog) contains many repeated Product entries (e.g., name "딸기주스", "무엘사") which can confuse readers and tests; either deduplicate the list or make duplication explicit: replace the repeated hardcoded Product instances with a small set of unique Product objects and if you need many items for paging generate copies programmatically (e.g., replicate uniqueProducts.mapIndexed { idx -> unique.copy(name = "${product.name} #$idx", ... ) } ) or add a clear comment above catalog explaining that repeats are intentional for pagination tests; update references to Product and catalog accordingly.app/src/main/java/woowacourse/shopping/repository/productRepository/CatalogProductRepository.kt (1)
7-17: ⚡ Quick win리포지토리 패턴의 캡슐화에 대해 고민해보셨나요?
Line 8에서
val catalog = MockCatalog이 public으로 노출되어 있습니다.다음을 생각해보세요:
- 리포지토리 패턴을 사용하는 목적은 무엇인가요?
- 외부에서
CatalogProductRepository.catalog에 직접 접근할 수 있다면 어떤 문제가 생길까요?- 구현 세부사항(MockCatalog)을 숨기는 것이 왜 중요할까요?
- 나중에
MockCatalog를 다른 데이터 소스로 교체할 때 영향을 받는 코드는 무엇일까요?힌트: Kotlin의 가시성 제한자(
private,internal)를 고려해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/repository/productRepository/CatalogProductRepository.kt` around lines 7 - 17, CatalogProductRepository currently exposes its implementation detail via the public val catalog = MockCatalog which breaks repository encapsulation; make the catalog reference non-public (e.g., private) so external code cannot access CatalogProductRepository.catalog or depend on MockCatalog directly, ensuring calls go through the repository API methods (getProducts, getProductById) and making future data-source swaps safe.app/build.gradle.kts (1)
58-66: ⚡ Quick win의존성 버전 관리 방식의 일관성을 고려해보세요.
일부 의존성들(lifecycle, Room, Coil)은 버전이 직접 하드코딩되어 있는 반면, 다른 의존성들은
libs.*패턴을 사용하고 있습니다.다음을 생각해보세요:
- 프로젝트에서 버전 카탈로그(
libs.versions.toml)를 사용하는 이유는 무엇일까요?- 일부 의존성만 하드코딩하면 어떤 문제가 발생할 수 있을까요?
- 모든 의존성을 동일한 방식으로 관리하면 어떤 이점이 있을까요?
힌트:
gradle/libs.versions.toml파일을 확인하고 Gradle 버전 카탈로그 문서를 참고해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/build.gradle.kts` around lines 58 - 66, Some dependencies in app/build.gradle.kts are hardcoded with versions (e.g., "androidx.lifecycle:lifecycle-viewmodel-compose:2.10.0", "io.coil-kt.coil3:coil-compose:3.4.0", "androidx.room:room-runtime:2.8.4", ksp("androidx.room:room-compiler:2.8.4")) while others use the version catalog (libs.*); move these hardcoded entries into the version catalog and replace them with libs references to ensure consistent version management. Update gradle/libs.versions.toml to add entries for lifecycle-viewmodel-compose, lifecycle-runtime-compose, coil-compose, coil-network-okhttp, room-runtime and room-compiler, then change the lines in app/build.gradle.kts to use implementation(libs.<your_key>) and ksp(libs.<room_compiler_key>) accordingly so all dependencies use the catalog.app/src/main/java/woowacourse/shopping/data/remote/ProductDto.kt (1)
15-20: ⚖️ Poor tradeoffUUID 생성 전략에 대해 생각해보셨나요?
Line 16에서 정수 ID를 UUID로 변환할 때
UUID.nameUUIDFromBytes(id.toString().toByteArray())를 사용하고 있습니다.다음을 고민해보세요:
- 정수를 문자열로 변환한 후 바이트 배열로 변환하는 이유는 무엇인가요?
- 서버의
id가 항상 정수라면, 더 직접적인 변환 방법은 없을까요?- 동일한 서버 ID에 대해 항상 동일한 UUID가 생성되는 것이 보장되나요?
- Type 3 UUID(MD5 기반)와 Type 5 UUID(SHA-1 기반)의 차이를 알고 계신가요?
생각해볼 점: UUID 생성 로직이 도메인 계층과 데이터 계층 사이의 경계에서 어떤 역할을 하는지 고민해보세요.
🤖 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/ProductDto.kt` around lines 15 - 20, ProductDto.toDomain currently derives productId via UUID.nameUUIDFromBytes(id.toString().toByteArray()), which is brittle (string/encoding dependent) and hides the UUID version; instead produce a deterministic, encoding-safe UUID from the numeric id by encoding the integer directly into bytes (e.g., use ByteBuffer to putLong/id bytes) before calling UUID.nameUUIDFromBytes, or better yet surface a UUID from the data layer (change ProductDto to carry a UUID or a namespaced conversion) so Product(productId) gets a clear, stable mapping; update ProductDto.toDomain to replace id.toString().toByteArray() with a direct numeric-to-bytes conversion (using ByteBuffer or equivalent) and document that this uses nameUUIDFromBytes (MD5 / v3) or implement a v5 (SHA-1) generator if you need type-5 UUIDs.app/src/main/java/woowacourse/shopping/data/entity/CartEntity.kt (1)
19-23: ⚡ Quick winRoom 엔티티의 기본값 동작을 이해해보세요.
Line 22의
addedAt: Long = System.currentTimeMillis()는 Kotlin 데이터 클래스 기본값으로, CartEntity 객체가 생성될 때(RoomCartRepository.addProduct에서) 평가됩니다. 이는 데이터베이스 삽입 시점이 아닌 인스턴스 생성 시점에 타임스탬프를 기록하게 됩니다.다음을 확인해보세요:
- Kotlin 데이터 클래스의 기본값과 Room의 데이터베이스 수준 기본값(
@ColumnInfo(defaultValue = ...))의 차이- 현재 구현에서
System.currentTimeMillis()가 평가되는 정확한 시점- Room에서 데이터베이스 삽입 시점의 타임스탬프를 기록하려면 어떤 방법들이 있을까요?
현재 구현은 정상 작동하며 addedAt으로 카트 아이템을 정렬하는 데 사용되고 있습니다. 테스트를 통해 현재 동작이 의도와 일치하는지 확인하거나, 필요시 데이터베이스 수준의 기본값 설정을 고려해보세요.
🤖 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/entity/CartEntity.kt` around lines 19 - 23, CartEntity's addedAt default (addedAt: Long = System.currentTimeMillis()) is evaluated at object construction (in RoomCartRepository.addProduct) not at DB insert; to ensure timestamp reflects insertion time either remove the Kotlin default and set addedAt = System.currentTimeMillis() explicitly in RoomCartRepository.addProduct when creating the entity, or keep the field nullable/absent and declare a DB-level default via `@ColumnInfo`(defaultValue = "...") (e.g., SQL CURRENT_TIMESTAMP or a millisecond expression) so Room/DB assigns the timestamp at insert; update CartEntity (remove the inline System.currentTimeMillis default or make it nullable) and modify RoomCartRepository.addProduct to populate addedAt if you choose the repository-side approach.app/src/main/java/woowacourse/shopping/CatalogViewModel.kt (2)
50-56: ⚡ Quick win
loadProducts의 페이지 카운터 증가 시점을 한 번 더 점검해 보세요.현재
loadProducts()는getProducts결과가 비어 있어도 (네트워크 실패로 빈 리스트가 와도)currentPage++가 무조건 실행됩니다. 다음 케이스를 생각해 보세요.
- 일시적 네트워크 오류로 한 번 빈 리스트가 반환되면, 다음 호출은 비어 있던 페이지를 건너뛰고 다음 페이지를 시도하게 됩니다. 의도된 동작일까요?
- 또한 중복 호출(스크롤 끝에서 빠르게 여러 번 트리거)이 들어오면 동일 페이지를 동시에 요청하지 않도록
isLoading플래그나Mutex같은 보호가 필요하지 않을지 한 번 검토해 보세요.🤖 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/CatalogViewModel.kt` around lines 50 - 56, loadProducts currently increments currentPage unconditionally after calling productRepository.getProducts, which causes skipped pages on empty/failed responses and allows duplicate concurrent fetches; modify loadProducts (and related state: currentPage, _products) to only increment currentPage when newProducts is non-empty (or when the response indicates success), and add a simple concurrency guard (e.g., an isLoading flag or a Mutex around the viewModelScope.launch block) to prevent simultaneous calls requesting the same page; ensure isLoading is set true before the network call and reset false in finally so duplicate rapid triggers are ignored.
31-42: 💤 Low value
getQuantity가catalogItems와 역할이 겹쳐 보입니다.이미
catalogItems가combine(products, cart)로 각 상품의quantity까지 함께 들고 있는데, 별도로getQuantity(id)가 또 노출되어 있습니다.
- Compose에서
getQuantity(id)를 직접 호출하면cart.value를 한 번 읽는 스냅샷이라 재구성 트리거가 자동으로 걸리지 않습니다. 의도와 다른 동작이 생길 수 있어요.- UI에서 수량이 필요한 위치는
CatalogItemUiState.quantity로 일관되게 가져오는 편이 안전해 보입니다.getQuantity를 정말 외부에서 따로 사용해야 하는 케이스가 있나요?Also applies to: 76-78
🤖 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/CatalogViewModel.kt` around lines 31 - 42, catalogItems already combines products and cart to expose per-item quantities via CatalogItemUiState.quantity, so remove or restrict the separate getQuantity(id) API that reads cart.value (which can produce stale Compose snapshots); either delete the public getQuantity(id) function or make it private/internal and update all callers to use catalogItems.map { it.quantity } (or access CatalogItemUiState.quantity) instead, ensuring UI reads quantity from catalogItems so recomposition is triggered correctly; update any usages of getQuantity in the codebase to reference CatalogItemUiState.quantity from catalogItems or pass the CatalogItemUiState into the composable.app/src/main/java/woowacourse/shopping/ProductDetailActivity.kt (1)
39-49: 💤 Low value최근 본 상품 추가와
lastViewedProduct도출 순서를 한 번 정리해 보세요.
lifecycleScope.launch { addRecentProduct(product) }는 비동기이고,recentProducts도collectAsStateWithLifecycle로 비동기 구독되기 때문에 화면 초기 컴포지션 시점의lastViewedProduct는 시점에 따라 달라질 수 있습니다. 현재 라인 49의firstOrNull { it.productId != productId }필터로 우회되긴 하지만, 다음 사항을 한 번 확인해 보세요.
recentProducts가 "최근에 본 순서"로 정렬되어 들어온다는 가정이 저장소 쪽 쿼리에서도 보장되나요?- 사용자가 처음 들어와 최근 본 상품이 비어 있을 때
ProductDetailScreen이nulllastViewedProduct를 적절히 처리하나요?🤖 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/ProductDetailActivity.kt` around lines 39 - 49, The order-of-operations is racey: you launch recentProductRepository.addRecentProduct(product) asynchronously while immediately subscribing to recentProducts via recentProducts.collectAsStateWithLifecycle and computing lastViewedProduct (the variable using firstOrNull { it.productId != productId }), so the UI can see inconsistent ordering or an empty list; ensure the repository guarantees "most recently viewed first" ordering at the data layer (verify/adjust the query in RecentProductRepository), or move the addRecentProduct call to a synchronous point before composing (or await its completion) so the initial collect sees the inserted item, and also make ProductDetailScreen handle a null lastViewedProduct safely (defensive null-checks) to cover the initial-empty case.app/src/main/java/woowacourse/shopping/repository/cartRepository/RoomCartRepository.kt (1)
26-33: 💤 Low value
init에서 시작된 collect 코루틴의 수명에 대해 한 번 생각해 보세요.
externalScope에서 무한히getCartItemsWithProduct()를 collect 합니다. 현재 호출 측(ShoppingApplication)에서 만든 스코프는 어디서도cancel()되지 않는데, 이는 의도된 동작인가요?
- 앱이 살아 있는 동안 항상 살아 있어야 하는 상태라면 명확히 의도를 주석으로 남겨두면 좋습니다.
- 테스트에서
RoomCartRepository인스턴스를 짧게 만들고 버리는 경우엔 스코프가 누적될 수 있습니다. 테스트 친화적인 구조가 필요할지 함께 검토해 보세요.app/src/test/java/woowacourse/shopping/repository/productRepository/HttpProductRepositoryTest.kt (1)
76-101: ⚡ Quick win페이지네이션 테스트가 실제로는 "첫 페이지의 size 제한"만 검증하고 있습니다.
테스트 이름은 "페이지네이션이 올바르게 동작한다"인데,
getProducts(0, 2)만 호출하고 있어 사실상 한 페이지의 슬라이스 길이만 확인하는 모양새가 되었어요. 다음 케이스 중 어떤 것이 더 의미 있는 검증이 될지 생각해 보세요.
getProducts(1, 2)호출 시상품3한 건만 반환되는지- 마지막 페이지를 넘어가는 인덱스(
getProducts(5, 2))에 대해 빈 리스트가 안전하게 반환되는지 (현재 구현의coerceAtMost보호가 동작하는지)또한 page 1을 검증하려면
MockWebServer가 동일 응답을 두 번 줄 수 있도록enqueue횟수도 함께 조정이 필요할 거예요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/woowacourse/shopping/repository/productRepository/HttpProductRepositoryTest.kt` around lines 76 - 101, The test only asserts page size for getProducts(0, 2) so update it to verify true pagination: enqueue the same MockResponse twice (using mockWebServer.enqueue(...) again) and call repository.getProducts(1, 2) asserting it returns only "상품3", then also call repository.getProducts(5, 2) (or another out-of-range page) and assert it returns an empty list to ensure the coerceAtMost protection behaves correctly; reference repository.getProducts and MockWebServer.enqueue when making these changes.app/src/main/java/woowacourse/shopping/repository/productRepository/HttpProductRepository.kt (1)
21-37: ⚡ Quick win네트워크 예외와 타임아웃 정책을 어떻게 처리할지 한 번 더 고민해 보세요.
현재
client.newCall(request).execute()가IOException을 던지면 그대로 호출부까지 전파됩니다.MainActivity토스트 흐름("네트워크 연결이 끊었습니다")과 연결시키려면, 저장소 레이어에서 실패를 어떤 타입으로 표현할지(예:Result<List<Product>>, 별도 sealed class 등) 결정해두는 편이 좋습니다.또한
OkHttpClient()기본 설정은 네트워크가 매우 나쁠 때 사용자 경험에 영향이 클 수 있으니, 연결/읽기 타임아웃을 명시하는 것도 함께 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/repository/productRepository/HttpProductRepository.kt` around lines 21 - 37, The getProducts implementation in HttpProductRepository currently lets IOExceptions from client.newCall(request).execute() propagate and doesn't express failure in the return type; change the repository API to return a failure-aware type (e.g., Result<List<Product>> or a sealed RepositoryResult) and update getProducts to catch network errors (IOException, SocketTimeoutException, etc.) and return a clear failure value instead of throwing; also configure the OkHttpClient used by HttpProductRepository (replace bare OkHttpClient() with OkHttpClient.Builder() and set sensible connectTimeout, readTimeout and callTimeout) so timeouts are enforced and mapped to the chosen failure representation by getProducts.app/src/main/java/woowacourse/shopping/ShoppingApplication.kt (1)
51-99: 💤 Low value대용량 JSON 문자열을 코드 내 템플릿으로 생성하는 부분을 분리해 보세요.
startMockWebServer()안에 60개 항목의 JSON을 문자열 템플릿으로 직접 만드는 로직이 메서드 절반 이상을 차지하고 있습니다. 다음 점을 생각해 보세요.
- 디스패처 설정/서버 시작 로직과 "샘플 데이터" 로직이 한 메서드에 섞여 있어 책임이 두 가지가 됐어요.
assets/나 별도MockProductsData파일로 분리하면, 데이터 수정 시Application을 건드리지 않아도 됩니다.지금은 학습용이라 동작에는 문제가 없지만, 한 번 더 분리할 여지가 있습니다.
🤖 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 51 - 99, Summary: startMockWebServer() mixes server setup with large inline JSON generation for the products variable; extract the sample-data generation to a separate module or asset. Fix: create a new helper (e.g., object MockProductsData with fun generateMockProductsJson(): String or place a static JSON under assets/) that produces the same JSON currently built into the products variable, then replace the inline template in startMockWebServer() with a single call to MockProductsData.generateMockProductsJson() (or load from assets) so startMockWebServer() only configures the MockWebServer/Dispatcher and starts the server. Update any imports and tests to read from the new MockProductsData or asset loader.app/src/main/java/woowacourse/shopping/ui/component/screen/CatalogScreen.kt (2)
135-152: 💤 Low value재사용 가능한 컴포넌트 추출을 고려해보세요.
장바구니 배지 UI (Lines 135-152)가 잘 구현되어 있습니다.
이 배지를 다른 화면에서도 사용할 가능성이 있나요? 만약 그렇다면:
- 별도의
@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/ui/component/screen/CatalogScreen.kt` around lines 135 - 152, Extract the inline badge UI in CatalogScreen into a reusable `@Composable` named CartBadge that takes at least a count: Int and optional modifier: Modifier = Modifier (and optional styling params like size: Dp, backgroundColor: Color, textStyle: TextStyle if desired); implement the same logic (only render when count != 0) and the same layout (Box with CircleShape, centered Text) inside CartBadge, then replace the inline block in CatalogScreen with a call to CartBadge(count = cartTotalAmount, modifier = Modifier.size(22.dp)) so other screens can reuse and test the badge independently.
58-61: 💤 Low value리스트 검색 로직의 효율성을 고려해보세요.
onIncrease콜백에서 매번catalogItems.find를 수행하고 있습니다.다음 질문들을 생각해보세요:
- 카탈로그 아이템이 수백 개라면 어떻게 될까요?
- 사용자가 버튼을 여러 번 빠르게 누르면?
- 검색 없이 필요한 정보를 전달할 방법이 있을까요?
ShoppingItem컴포넌트에 어떤 정보를 미리 전달할 수 있을지 생각해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/component/screen/CatalogScreen.kt` around lines 58 - 61, The onIncrease handler is repeatedly scanning catalogItems with catalogItems.find to locate a product by id, which is inefficient and can be slow under many items or rapid clicks; instead, pass the necessary product (or productId plus reference) directly into the ShoppingItem so the onIncrease callback gets the product immediately. Update the ShoppingItem usage to include the product (or a lightweight identifier object) and change the onIncrease signature to accept that product (or object) and call viewModel.addProductToCart(product) directly, removing the catalogItems.find lookup in the onIncrease lambda.app/src/main/java/woowacourse/shopping/MainActivity.kt (2)
46-67: ⚡ Quick win초기 방출을 건너뛰는 더 나은 방법을 고려해보세요.
isInitial플래그를 사용하여 첫 번째 방출을 건너뛰는 것은 작동하지만, Flow API에는 이를 더 명시적으로 처리하는 연산자들이 있습니다.이러한 패턴의 문제점을 생각해보세요:
- 상태 변수를 수동으로 관리해야 합니다
- 의도가 코드에서 명확하지 않습니다
Kotlin Flow는 이런 상황을 위한 연산자를 제공합니다.
drop,distinctUntilChanged같은 연산자들을 살펴보시겠어요?🤖 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/MainActivity.kt` around lines 46 - 67, Replace the manual isInitial flag in the LaunchedEffect collecting networkMonitor.isConnected with Flow operators: apply drop(1) to skip the initial emission (and consider distinctUntilChanged() if you only want changes), then collect the resulting flow; update the code that references networkMonitor.isConnected.collect to use networkMonitor.isConnected.drop(1).collect (or networkMonitor.isConnected.drop(1).distinctUntilChanged().collect) and remove the isInitial variable and its conditional branch.
31-43: ⚖️ Poor tradeoffViewModel Factory 생성 방식을 개선할 수 있습니다.
인라인으로 익명
ViewModelProvider.Factory를 생성하고@Suppress("UNCHECKED_CAST")를 사용하는 것은 일반적인 패턴이지만, 다음과 같은 단점이 있습니다:
- 여러 Activity/Fragment에서 재사용할 수 없습니다
- 타입 안전성을 약화시킵니다
더 나은 접근 방식들을 고려해보세요:
- 재사용 가능한 Factory 클래스를 별도로 정의
- Hilt 같은 DI 프레임워크 사용 검토
현재 구조에서 Factory를 재사용할 다른 곳이 있나요?
🤖 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/MainActivity.kt` around lines 31 - 43, Extract the anonymous ViewModelProvider.Factory into a reusable class (e.g., CatalogViewModelFactory) that accepts productRepository, cartRepository, and recentProductRepository in its constructor and implements ViewModelProvider.Factory; implement create(modelClass: Class<T>) to check modelClass.isAssignableFrom(CatalogViewModel::class.java) and return a new CatalogViewModel instance (or throw IllegalArgumentException for unknown classes) so you can remove `@Suppress`("UNCHECKED_CAST"); then update the viewModel(...) call to pass an instance of CatalogViewModelFactory instead of the inline anonymous object to enable reuse and stronger type-safety.app/src/main/java/woowacourse/shopping/ui/stateholder/CartStateHolder.kt (1)
24-33: ⚖️ Poor tradeoffFlow 수집과 상태 업데이트 로직을 검토해보세요.
init블록에서cartRepository.cartFlow를 수집하고 있습니다.다음 사항들을 고려해보셨나요?
coroutineScope가 취소되거나 완료되면 어떻게 되나요?- 이 StateHolder가 여러 번 생성되고 파괴되는 경우는?
- Flow 수집이 예외를 발생시키면?
상태 관리 컴포넌트의 생명주기와 코루틴 스코프의 관계를 명확히 이해하고 있는지 확인해보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/stateholder/CartStateHolder.kt` around lines 24 - 33, The init block starts a collector on cartRepository.cartFlow using a raw coroutineScope.launch but doesn't handle scope lifecycle, cancellations, or flow exceptions; change CartStateHolder to create and own a CoroutineScope (e.g., CoroutineScope(SupervisorJob() + Dispatchers.Main)) as a property, replace the launch with cartRepository.cartFlow.onEach { ... }.catch { /* log/error-handle */ }.launchIn(yourScope) so exceptions are handled and the flow is tied to that scope, and add a clear/close/dispose method that cancels the scope (or cancel any existing scope before recreating) to ensure proper cleanup when the StateHolder is destroyed or re-created.
🤖 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/java/woowacourse/shopping/data/dao/ShoppingDao.kt`:
- Around line 21-22: The getAllProducts() DAO method currently returns a
synchronous List<CatalogEntity>, which can cause Room to throw on the main
thread and is inconsistent with other DAO methods; change its signature to
return kotlinx.coroutines.flow.Flow<List<CatalogEntity>> (i.e., fun
getAllProducts(): Flow<List<CatalogEntity>>) so it uses Room's recommended
asynchronous reactive pattern consistent with getCartItemsWithProduct() and
getRecentProductsWithDetail(), and ensure the Flow import is added.
In `@app/src/main/java/woowacourse/shopping/domain/CartProduct.kt`:
- Line 14: Fix the spelling in the validation message inside CartProduct (the
require call that checks amount >= 0) by replacing "수량은 0 이상이여야 합니다." with the
correct phrase "수량은 0 이상이어야 합니다." so the error message uses "이어야" instead of
"이여야".
- Line 22: The addQuantity method in CartProduct currently lacks validation and
will allow negative requestedAmount (or reduce amount if negative); update
CartProduct.addQuantity(requestedAmount: Int) to validate requestedAmount (e.g.,
require(requestedAmount >= 0) with a clear message) or, if negative inputs
should be tolerated, normalize the result using amount +
requestedAmount.coerceAtLeast(-amount) or finalAmount.coerceAtLeast(0) to
prevent negative totals; ensure the chosen approach is consistent with
decreaseQuantity and the existing require on line 14 and the coerceAtLeast(0)
logic on line 18 so both methods enforce the same invariants.
In `@app/src/main/java/woowacourse/shopping/domain/CartProducts.kt`:
- Around line 31-43: decreaseQuantityOfCartProduct currently only reduces
quantity and keeps items with amount==0; after calling
CartProduct.decreaseQuantity inside decreaseQuantityOfCartProduct you should
remove any cart products whose quantity became 0 so uniqueItemCount/UI stay
correct. Locate decreaseQuantityOfCartProduct (and use findSameProduct and
CartProduct.decreaseQuantity to find behavior), produce the updatedProducts by
mapping then filtering out products with amount <= 0 (or otherwise
skipping/omitting null/zero results), and return new
CartProducts(updatedProducts) so zero-quantity items are not retained.
In `@app/src/main/java/woowacourse/shopping/ProductDetailActivity.kt`:
- Around line 32-37: ProductDetailActivity calls
productRepository.getProductById on the main thread, which blocks because
HttpProductRepository uses runBlocking; fix by avoiding synchronous network
calls on onCreate: either (A) pass required Product data via the Intent from the
previous Activity or read from a local cache/DB exposed by
ShoppingApplication.productRepository so no network call is needed in
ProductDetailActivity, or (B) make getProductById a suspend function in the
repository and update call sites to invoke it inside lifecycleScope.launch
within ProductDetailActivity (use lifecycleScope.launch { val product =
productRepository.getProductById(productId); if (product == null) finish() else
bindUI(product) }) so the network request does not block the UI thread; update
ShoppingApplication and all repository implementations accordingly (referenced
symbols: ProductDetailActivity, ShoppingApplication, productRepository,
getProductById, lifecycleScope.launch).
In
`@app/src/main/java/woowacourse/shopping/repository/cartRepository/InMemoryCartRepository.kt`:
- Around line 17-29: The current read-modify-write on _cartFlow in addProduct,
decreaseProduct, and removeProduct is not atomic and can lose concurrent
updates; change each to use MutableStateFlow.update (import
kotlinx.coroutines.flow.update) so the transformation is applied atomically,
e.g. replace `_cartFlow.value = _cartFlow.value.addProduct(...)` with
`_cartFlow.update { it.addProduct(...) }` and do the same for decreaseProduct
(it.decreaseProduct(...)) and removeProduct (it.removeProduct(...)).
In
`@app/src/main/java/woowacourse/shopping/repository/cartRepository/RoomCartRepository.kt`:
- Around line 35-57: The comment points out that addProduct and decreaseProduct
are not atomic despite the "race condition" comment; wrap the multi-step logic
into a single Room transaction or replace the sequence with an atomic DAO
operation (e.g., an `@Query` using SQL ON CONFLICT DO UPDATE or an `@Upsert-like`
method) so concurrent coroutines cannot lost-update; specifically, modify
RoomCartRepository.addProduct and decreaseProduct to call a new DAO method that
performs the insert-or-increment and decrement-or-delete atomically (instead of
calling dao.insertProduct, dao.getCartItemAmount, dao.updateCartQuantity,
dao.insertCartItem, dao.deleteCartItem separately), or annotate the repository
methods with `@Transaction` and move the logic into a single transactional DAO
method to ensure correctness.
In
`@app/src/main/java/woowacourse/shopping/repository/productRepository/HttpProductRepository.kt`:
- Around line 39-41: getProductById currently blocks the caller with runBlocking
and only searches the first page via getProducts(0, 20), causing missed IDs and
main-thread ANRs; change the repository to either (A) implement and call a
dedicated single-item fetch (e.g., add a GET /products/{id} client method and
make getProductById use it) or (B) make getProductById a suspend function
(remove runBlocking) and update callers (e.g., ProductDetailActivity.onCreate)
to call it from a coroutine (lifecycleScope.launch), ensuring no blocking and
correct retrieval beyond the first page; update signatures and usages of
getProductById/getProducts accordingly.
In `@app/src/main/java/woowacourse/shopping/ShoppingApplication.kt`:
- Around line 117-122: There is a race between starting MockWebServer and
creating productRepository: replace the asynchronous thread {
mockWebServer.start(12345) } with a synchronous start or synchronize exposure of
productRepository so it is only created after start() completes and any
exceptions are surfaced; specifically either call mockWebServer.start(12345)
directly on the calling thread (since it is fast and blocking) or use a
synchronization primitive (e.g., CountDownLatch or a start-complete callback) so
that productRepository = HttpProductRepository("http://localhost:12345/") (and
MainActivity.onCreate page load) only run after mockWebServer.start finishes and
its exceptions are logged/handled.
- Around line 8-11: The production source imports of MockWebServer
(okhttp3.mockwebserver.Dispatcher, MockResponse, MockWebServer, RecordedRequest)
must be removed from ShoppingApplication.kt; either move the MockWebServer usage
into a debug-only module/config by declaring the dependency as
debugImplementation in Gradle and relocating any mock setup into a debug-only
class, or replace the runtime mock with a proper test double by implementing an
InMemoryProductRepository that implements ProductRepository and wiring that into
the application for non-production builds so the production APK no longer
depends on okhttp3.mockwebserver.
- Around line 124-127: The code relies on Application.onTerminate to clean up
mockWebServer and cartRepository's CoroutineScope, but onTerminate is not called
on real devices; move mockWebServer out of production Application lifecycle
(only start/stop it in tests or in an explicitly controlled test harness) and
stop creating long‑lived uncancelled scopes in Application. Specifically, remove
mockWebServer startup from ShoppingApplication.onCreate/onTerminate and run it
only in test setup/teardown, and refactor the cartRepository so its
CoroutineScope(SupervisorJob() + Dispatchers.IO) is not an unmanaged global:
make the scope owned by a lifecycle-aware component (e.g., created in a
ViewModel using viewModelScope, or wrap repository with a close() / shutdown()
method that cancels the SupervisorJob, or accept an injected CoroutineScope),
then ensure callers cancel/close the repository when no longer needed instead of
relying on onTerminate.
In `@app/src/main/java/woowacourse/shopping/ui/stateholder/CartStateHolder.kt`:
- Around line 43-65: The handlers
onIncreaseProduct/onDecreaseProduct/onDeleteProduct call
cartRepository.addProduct/decreaseProduct/removeProduct which perform multiple
DB reads/writes and can race; fix by making repository operations atomic and
serialized: either mark the multi-step Room DAO methods or repository methods
with `@Transaction` so Room performs the read/update/insert as one transaction, or
add a coroutine Mutex inside RoomCartRepository and InMemoryCartRepository to
guard the multi-step methods (acquire lock around
addProduct/decreaseProduct/removeProduct and update MutableStateFlow.value under
the same lock); add a concurrent test that launches multiple coroutines calling
onIncreaseProduct/onDecreaseProduct to reproduce the issue and verify the chosen
fix prevents lost updates.
In
`@app/src/main/java/woowacourse/shopping/ui/stateholder/CatalogScreenStateHolder.kt`:
- Around line 76-80: loadProducts() blocks the main thread via runBlocking;
instead, call the suspend productRepository.getProducts(...) from the existing
coroutineScope by launching a coroutine (e.g., coroutineScope.launch { ... })
and update _catalog.value with the result inside that coroutine. Replace the
runBlocking block in loadProducts() with a coroutineScope.launch that invokes
productRepository.getProducts(recentItemIndex, MAX_PRODUCT), appends to
_catalog, and add basic error handling (try/catch) to surface/log failures; keep
using the repository's internal Dispatchers.IO so the call remains off the main
thread.
- Around line 89-95: The retainCatalogScreenStateHolder currently captures a
composition-tied scope from rememberCoroutineScope() outside retain, causing the
retained CatalogScreenStateHolder to reference a cancelled scope; fix by
removing rememberCoroutineScope() and instead create and manage a long-lived
scope inside the retain block (e.g., CoroutineScope(SupervisorJob() +
Dispatchers.Main)) when constructing CatalogScreenStateHolder (coroutineScope
parameter), and ensure you cancel that scope in retain's dispose/onRelease
callback so cartFlow collection and onIncrease()/onDecrease() coroutines run on
a scope with the same lifetime as the retained holder.
In `@app/src/test/java/woowacourse/shopping/domain/CartProductTest.kt`:
- Around line 10-14: Rename the failing test's display name in CartProductTest
so it matches the assertion: change the function name string from "개수는 0 이상이라면
예외가 발생한다" to a correct description such as "개수는 0 미만이라면 예외가 발생한다" (or "개수가 음수라면
예외가 발생한다"); keep the test body as-is which constructs CartProduct(product =
Product(...), amount = -1) and asserts an IllegalArgumentException to ensure
CartProduct validation behavior remains tested and clear.
In `@app/src/test/java/woowacourse/shopping/domain/CartTest.kt`:
- Around line 44-50: The test "겹치는 상품을 추가하면 전체 개수가 증가한다" currently creates two
Products with different productIds so it validates adding a new item instead of
merging; update the test to use the same product identity when calling
Cart.addProduct so merge logic is exercised — e.g., construct the original
CartProduct and the newProduct with the same productId (or reuse the same
Product instance) before calling cart.addProduct(...), then assert via
Cart.getTotalQuantity() that quantities merged as expected; refer to Cart,
CartProducts, CartProduct, Product, addProduct and getTotalQuantity when making
the change.
---
Nitpick comments:
In `@app/build.gradle.kts`:
- Around line 58-66: Some dependencies in app/build.gradle.kts are hardcoded
with versions (e.g., "androidx.lifecycle:lifecycle-viewmodel-compose:2.10.0",
"io.coil-kt.coil3:coil-compose:3.4.0", "androidx.room:room-runtime:2.8.4",
ksp("androidx.room:room-compiler:2.8.4")) while others use the version catalog
(libs.*); move these hardcoded entries into the version catalog and replace them
with libs references to ensure consistent version management. Update
gradle/libs.versions.toml to add entries for lifecycle-viewmodel-compose,
lifecycle-runtime-compose, coil-compose, coil-network-okhttp, room-runtime and
room-compiler, then change the lines in app/build.gradle.kts to use
implementation(libs.<your_key>) and ksp(libs.<room_compiler_key>) accordingly so
all dependencies use the catalog.
In `@app/src/main/java/woowacourse/shopping/CatalogViewModel.kt`:
- Around line 50-56: loadProducts currently increments currentPage
unconditionally after calling productRepository.getProducts, which causes
skipped pages on empty/failed responses and allows duplicate concurrent fetches;
modify loadProducts (and related state: currentPage, _products) to only
increment currentPage when newProducts is non-empty (or when the response
indicates success), and add a simple concurrency guard (e.g., an isLoading flag
or a Mutex around the viewModelScope.launch block) to prevent simultaneous calls
requesting the same page; ensure isLoading is set true before the network call
and reset false in finally so duplicate rapid triggers are ignored.
- Around line 31-42: catalogItems already combines products and cart to expose
per-item quantities via CatalogItemUiState.quantity, so remove or restrict the
separate getQuantity(id) API that reads cart.value (which can produce stale
Compose snapshots); either delete the public getQuantity(id) function or make it
private/internal and update all callers to use catalogItems.map { it.quantity }
(or access CatalogItemUiState.quantity) instead, ensuring UI reads quantity from
catalogItems so recomposition is triggered correctly; update any usages of
getQuantity in the codebase to reference CatalogItemUiState.quantity from
catalogItems or pass the CatalogItemUiState into the composable.
In `@app/src/main/java/woowacourse/shopping/data/entity/CartEntity.kt`:
- Around line 19-23: CartEntity's addedAt default (addedAt: Long =
System.currentTimeMillis()) is evaluated at object construction (in
RoomCartRepository.addProduct) not at DB insert; to ensure timestamp reflects
insertion time either remove the Kotlin default and set addedAt =
System.currentTimeMillis() explicitly in RoomCartRepository.addProduct when
creating the entity, or keep the field nullable/absent and declare a DB-level
default via `@ColumnInfo`(defaultValue = "...") (e.g., SQL CURRENT_TIMESTAMP or a
millisecond expression) so Room/DB assigns the timestamp at insert; update
CartEntity (remove the inline System.currentTimeMillis default or make it
nullable) and modify RoomCartRepository.addProduct to populate addedAt if you
choose the repository-side approach.
In `@app/src/main/java/woowacourse/shopping/data/entity/RecentProductEntity.kt`:
- Line 10: The RecentProductEntity currently sets viewedAt: Long =
System.currentTimeMillis() inside the entity; remove the implicit time
generation from RecentProductEntity (viewedAt) and require callers to supply the
timestamp instead (or add a factory/constructor overload that accepts a
Clock/Instant provider), then set timestamps at the creation point in the
repository/DAO (or test helper) so tests can inject deterministic times and
production code can use System.currentTimeMillis() there; update usages that
construct RecentProductEntity to pass an explicit viewedAt (or to use the new
factory with an injectable clock).
In `@app/src/main/java/woowacourse/shopping/data/remote/ProductDto.kt`:
- Around line 15-20: ProductDto.toDomain currently derives productId via
UUID.nameUUIDFromBytes(id.toString().toByteArray()), which is brittle
(string/encoding dependent) and hides the UUID version; instead produce a
deterministic, encoding-safe UUID from the numeric id by encoding the integer
directly into bytes (e.g., use ByteBuffer to putLong/id bytes) before calling
UUID.nameUUIDFromBytes, or better yet surface a UUID from the data layer (change
ProductDto to carry a UUID or a namespaced conversion) so Product(productId)
gets a clear, stable mapping; update ProductDto.toDomain to replace
id.toString().toByteArray() with a direct numeric-to-bytes conversion (using
ByteBuffer or equivalent) and document that this uses nameUUIDFromBytes (MD5 /
v3) or implement a v5 (SHA-1) generator if you need type-5 UUIDs.
In `@app/src/main/java/woowacourse/shopping/MainActivity.kt`:
- Around line 46-67: Replace the manual isInitial flag in the LaunchedEffect
collecting networkMonitor.isConnected with Flow operators: apply drop(1) to skip
the initial emission (and consider distinctUntilChanged() if you only want
changes), then collect the resulting flow; update the code that references
networkMonitor.isConnected.collect to use
networkMonitor.isConnected.drop(1).collect (or
networkMonitor.isConnected.drop(1).distinctUntilChanged().collect) and remove
the isInitial variable and its conditional branch.
- Around line 31-43: Extract the anonymous ViewModelProvider.Factory into a
reusable class (e.g., CatalogViewModelFactory) that accepts productRepository,
cartRepository, and recentProductRepository in its constructor and implements
ViewModelProvider.Factory; implement create(modelClass: Class<T>) to check
modelClass.isAssignableFrom(CatalogViewModel::class.java) and return a new
CatalogViewModel instance (or throw IllegalArgumentException for unknown
classes) so you can remove `@Suppress`("UNCHECKED_CAST"); then update the
viewModel(...) call to pass an instance of CatalogViewModelFactory instead of
the inline anonymous object to enable reuse and stronger type-safety.
In `@app/src/main/java/woowacourse/shopping/MockCatalog.kt`:
- Around line 11-308: The catalog list (variable catalog) contains many repeated
Product entries (e.g., name "딸기주스", "무엘사") which can confuse readers and tests;
either deduplicate the list or make duplication explicit: replace the repeated
hardcoded Product instances with a small set of unique Product objects and if
you need many items for paging generate copies programmatically (e.g., replicate
uniqueProducts.mapIndexed { idx -> unique.copy(name = "${product.name} #$idx",
... ) } ) or add a clear comment above catalog explaining that repeats are
intentional for pagination tests; update references to Product and catalog
accordingly.
In `@app/src/main/java/woowacourse/shopping/NetworkMonitor.kt`:
- Around line 26-28: Replace the nullable boolean expression using `== true`
with an explicit null check to clarify intent: where `activeNetwork`,
`capabilities`, and the `trySend(...)` call are used, change the condition from
`capabilities?.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) ==
true` to an explicit form such as `capabilities != null &&
capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)` so the
`trySend(...)` clearly receives a non-null boolean result.
In `@app/src/main/java/woowacourse/shopping/ProductDetailActivity.kt`:
- Around line 39-49: The order-of-operations is racey: you launch
recentProductRepository.addRecentProduct(product) asynchronously while
immediately subscribing to recentProducts via
recentProducts.collectAsStateWithLifecycle and computing lastViewedProduct (the
variable using firstOrNull { it.productId != productId }), so the UI can see
inconsistent ordering or an empty list; ensure the repository guarantees "most
recently viewed first" ordering at the data layer (verify/adjust the query in
RecentProductRepository), or move the addRecentProduct call to a synchronous
point before composing (or await its completion) so the initial collect sees the
inserted item, and also make ProductDetailScreen handle a null lastViewedProduct
safely (defensive null-checks) to cover the initial-empty case.
In
`@app/src/main/java/woowacourse/shopping/repository/productRepository/CatalogProductRepository.kt`:
- Around line 7-17: CatalogProductRepository currently exposes its
implementation detail via the public val catalog = MockCatalog which breaks
repository encapsulation; make the catalog reference non-public (e.g., private)
so external code cannot access CatalogProductRepository.catalog or depend on
MockCatalog directly, ensuring calls go through the repository API methods
(getProducts, getProductById) and making future data-source swaps safe.
In
`@app/src/main/java/woowacourse/shopping/repository/productRepository/HttpProductRepository.kt`:
- Around line 21-37: The getProducts implementation in HttpProductRepository
currently lets IOExceptions from client.newCall(request).execute() propagate and
doesn't express failure in the return type; change the repository API to return
a failure-aware type (e.g., Result<List<Product>> or a sealed RepositoryResult)
and update getProducts to catch network errors (IOException,
SocketTimeoutException, etc.) and return a clear failure value instead of
throwing; also configure the OkHttpClient used by HttpProductRepository (replace
bare OkHttpClient() with OkHttpClient.Builder() and set sensible connectTimeout,
readTimeout and callTimeout) so timeouts are enforced and mapped to the chosen
failure representation by getProducts.
In
`@app/src/main/java/woowacourse/shopping/repository/productRepository/ProductRepository.kt`:
- Around line 7-8: The interface mixes async and sync APIs—getProducts is
suspend while getProductById is not—so make the behavior consistent: change
getProductById to a suspend function (update its signature in
ProductRepository.kt to suspend fun getProductById(id: UUID): Product?) and then
update all implementations and callers of ProductRepository.getProductById to
use coroutine contexts (or wrap blocking calls with withContext(Dispatchers.IO)
inside implementations) so database/HTTP access remains non-blocking and
consistent with getProducts.
In `@app/src/main/java/woowacourse/shopping/ShoppingApplication.kt`:
- Around line 51-99: Summary: startMockWebServer() mixes server setup with large
inline JSON generation for the products variable; extract the sample-data
generation to a separate module or asset. Fix: create a new helper (e.g., object
MockProductsData with fun generateMockProductsJson(): String or place a static
JSON under assets/) that produces the same JSON currently built into the
products variable, then replace the inline template in startMockWebServer() with
a single call to MockProductsData.generateMockProductsJson() (or load from
assets) so startMockWebServer() only configures the MockWebServer/Dispatcher and
starts the server. Update any imports and tests to read from the new
MockProductsData or asset loader.
In `@app/src/main/java/woowacourse/shopping/ui/component/item/AddCircleButton.kt`:
- Around line 31-35: The Icon currently sets contentDescription = null which
makes the button inaccessible; update the AddCircleButton composable (in
AddCircleButton.kt) to provide a meaningful contentDescription (e.g., use
stringResource(R.string.add_to_cart) or accept a contentDescription parameter)
instead of null, and wire that string resource (localized) into the Icon(painter
= painterResource(R.drawable.ic_add), contentDescription = /* meaningful string
*/) so screen readers will announce the button purpose.
- Around line 22-29: Replace the clickable Surface in AddCircleButton.kt with a
proper Material3 button component (e.g., IconButton or FloatingActionButton)
instead of applying .clickable to Surface: keep the onAdd callback as the
button's onClick, move size/shape/color/shadowElevation into the chosen
component's parameters or its Modifier, and remove the manual .clickable
modifier on Surface; this ensures built-in ripple, focus, and accessibility
behavior from IconButton/FloatingActionButton and preserves the current visuals
while improving affordance and accessibility.
In `@app/src/main/java/woowacourse/shopping/ui/component/screen/CatalogScreen.kt`:
- Around line 135-152: Extract the inline badge UI in CatalogScreen into a
reusable `@Composable` named CartBadge that takes at least a count: Int and
optional modifier: Modifier = Modifier (and optional styling params like size:
Dp, backgroundColor: Color, textStyle: TextStyle if desired); implement the same
logic (only render when count != 0) and the same layout (Box with CircleShape,
centered Text) inside CartBadge, then replace the inline block in CatalogScreen
with a call to CartBadge(count = cartTotalAmount, modifier =
Modifier.size(22.dp)) so other screens can reuse and test the badge
independently.
- Around line 58-61: The onIncrease handler is repeatedly scanning catalogItems
with catalogItems.find to locate a product by id, which is inefficient and can
be slow under many items or rapid clicks; instead, pass the necessary product
(or productId plus reference) directly into the ShoppingItem so the onIncrease
callback gets the product immediately. Update the ShoppingItem usage to include
the product (or a lightweight identifier object) and change the onIncrease
signature to accept that product (or object) and call
viewModel.addProductToCart(product) directly, removing the catalogItems.find
lookup in the onIncrease lambda.
In `@app/src/main/java/woowacourse/shopping/ui/stateholder/CartStateHolder.kt`:
- Around line 24-33: The init block starts a collector on
cartRepository.cartFlow using a raw coroutineScope.launch but doesn't handle
scope lifecycle, cancellations, or flow exceptions; change CartStateHolder to
create and own a CoroutineScope (e.g., CoroutineScope(SupervisorJob() +
Dispatchers.Main)) as a property, replace the launch with
cartRepository.cartFlow.onEach { ... }.catch { /* log/error-handle */
}.launchIn(yourScope) so exceptions are handled and the flow is tied to that
scope, and add a clear/close/dispose method that cancels the scope (or cancel
any existing scope before recreating) to ensure proper cleanup when the
StateHolder is destroyed or re-created.
In `@app/src/test/java/woowacourse/shopping/domain/CartProductTest.kt`:
- Line 11: The test uses an incorrect spacing with the generic on assertThrows:
change the usage in CartProductTest.kt from "assertThrows
<IllegalArgumentException>" to the conventional
"assertThrows<IllegalArgumentException>" (attach the generic directly to the
assertThrows call) so the generic type parameter is written without a space.
In
`@app/src/test/java/woowacourse/shopping/repository/productRepository/HttpProductRepositoryTest.kt`:
- Around line 76-101: The test only asserts page size for getProducts(0, 2) so
update it to verify true pagination: enqueue the same MockResponse twice (using
mockWebServer.enqueue(...) again) and call repository.getProducts(1, 2)
asserting it returns only "상품3", then also call repository.getProducts(5, 2) (or
another out-of-range page) and assert it returns an empty list to ensure the
coerceAtMost protection behaves correctly; reference repository.getProducts and
MockWebServer.enqueue when making these changes.
🪄 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: 49c501fb-8688-49a3-a907-994ef38c1ddd
⛔ Files ignored due to path filters (1)
app/src/main/res/drawable/ic_plus.pngis excluded by!**/*.png
📒 Files selected for processing (54)
README.mdapp/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/woowacourse/shopping/CartActivity.ktapp/src/main/java/woowacourse/shopping/CatalogViewModel.ktapp/src/main/java/woowacourse/shopping/MainActivity.ktapp/src/main/java/woowacourse/shopping/MockCatalog.ktapp/src/main/java/woowacourse/shopping/NetworkMonitor.ktapp/src/main/java/woowacourse/shopping/ProductDetailActivity.ktapp/src/main/java/woowacourse/shopping/ShoppingApplication.ktapp/src/main/java/woowacourse/shopping/data/Mapper.ktapp/src/main/java/woowacourse/shopping/data/dao/ShoppingDao.ktapp/src/main/java/woowacourse/shopping/data/db/CartItemWithProduct.ktapp/src/main/java/woowacourse/shopping/data/db/RecentProductWithProduct.ktapp/src/main/java/woowacourse/shopping/data/db/ShoppingDatabase.ktapp/src/main/java/woowacourse/shopping/data/db/TypeConverters.ktapp/src/main/java/woowacourse/shopping/data/entity/CartEntity.ktapp/src/main/java/woowacourse/shopping/data/entity/CatalogEntity.ktapp/src/main/java/woowacourse/shopping/data/entity/RecentProductEntity.ktapp/src/main/java/woowacourse/shopping/data/remote/ProductDto.ktapp/src/main/java/woowacourse/shopping/domain/Cart.ktapp/src/main/java/woowacourse/shopping/domain/CartProduct.ktapp/src/main/java/woowacourse/shopping/domain/CartProducts.ktapp/src/main/java/woowacourse/shopping/domain/Product.ktapp/src/main/java/woowacourse/shopping/repository/cartRepository/CartRepository.ktapp/src/main/java/woowacourse/shopping/repository/cartRepository/InMemoryCartRepository.ktapp/src/main/java/woowacourse/shopping/repository/cartRepository/RoomCartRepository.ktapp/src/main/java/woowacourse/shopping/repository/productRepository/CatalogProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/productRepository/HttpProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/productRepository/ProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/productRepository/RecentProductRepository.ktapp/src/main/java/woowacourse/shopping/repository/productRepository/RoomRecentProductRepository.ktapp/src/main/java/woowacourse/shopping/ui/component/item/AddCircleButton.ktapp/src/main/java/woowacourse/shopping/ui/component/item/AmountModifyButton.ktapp/src/main/java/woowacourse/shopping/ui/component/item/CartItem.ktapp/src/main/java/woowacourse/shopping/ui/component/item/RecentItem.ktapp/src/main/java/woowacourse/shopping/ui/component/item/RecentlyViewedSection.ktapp/src/main/java/woowacourse/shopping/ui/component/item/ShoppingItem.ktapp/src/main/java/woowacourse/shopping/ui/component/screen/CartScreen.ktapp/src/main/java/woowacourse/shopping/ui/component/screen/CatalogScreen.ktapp/src/main/java/woowacourse/shopping/ui/component/screen/ProductDetailScreen.ktapp/src/main/java/woowacourse/shopping/ui/stateholder/CartStateHolder.ktapp/src/main/java/woowacourse/shopping/ui/stateholder/CatalogScreenStateHolder.ktapp/src/main/java/woowacourse/shopping/ui/theme/Theme.ktapp/src/main/res/xml/network_security_config.xmlapp/src/test/java/woowacourse/shopping/CartProductTest.ktapp/src/test/java/woowacourse/shopping/CartTest.ktapp/src/test/java/woowacourse/shopping/MockCatalogTest.ktapp/src/test/java/woowacourse/shopping/domain/CartProductTest.ktapp/src/test/java/woowacourse/shopping/domain/CartProductsTest.ktapp/src/test/java/woowacourse/shopping/domain/CartTest.ktapp/src/test/java/woowacourse/shopping/domain/ProductTest.ktapp/src/test/java/woowacourse/shopping/repository/productRepository/HttpProductRepositoryTest.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (2)
- app/src/test/java/woowacourse/shopping/CartTest.kt
- app/src/test/java/woowacourse/shopping/CartProductTest.kt
Comment on lines
+21
to
+22
| @Query("SELECT * FROM products") | ||
| fun getAllProducts(): List<CatalogEntity> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: getAllProducts 사용처 확인
rg -nP --type=kt -C3 '\bgetAllProducts\s*\(' -g '!**/ShoppingDao.kt'Repository: woowacourse/android-shopping-cart
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Fix: Use Kotlin file extension instead of type name
# Find usage of getAllProducts
rg -nP 'getAllProducts' --type-add 'kt:*.kt' -t kt -C3Repository: woowacourse/android-shopping-cart
Length of output: 791
🏁 Script executed:
#!/bin/bash
# Read the ShoppingDao.kt file to see all method definitions
cat app/src/main/java/woowacourse/shopping/data/dao/ShoppingDao.ktRepository: woowacourse/android-shopping-cart
Length of output: 1774
🏁 Script executed:
#!/bin/bash
# Search for other query methods to understand the pattern
rg -nP '@Query' app/src/main/java/woowacourse/shopping/data/dao/ShoppingDao.kt -A2Repository: woowacourse/android-shopping-cart
Length of output: 899
🏁 Script executed:
#!/bin/bash
# Search for getAllProducts calls across the entire codebase
rg -nP 'getAllProducts' --type-add 'kt:*.kt' -t kt --iglob '!**/ShoppingDao.kt'Repository: woowacourse/android-shopping-cart
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Also check for any test files or usage in Java files
rg -nP 'getAllProducts' --type-add 'kt:*.kt' -t kt -t javaRepository: woowacourse/android-shopping-cart
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Double-check for any dynamic/reflection-based usage patterns
rg -nP 'getAllProducts|Dao\[|getMethod' --type-add 'kt:*.kt' -t kt -t java | head -20Repository: woowacourse/android-shopping-cart
Length of output: 189
🌐 Web query:
Room database synchronous query main thread blocking behavior
💡 Result:
By default, synchronous (non-suspending, non-LiveData/Flow) Room DAO queries throw IllegalStateException if called on the main thread, preventing UI blocking: "Cannot access database on the main thread since it may potentially lock the UI for a long period of time."[1][2][3] This check enforces off-main-thread execution.[4][5] If.allowMainThreadQueries is enabled on the RoomDatabase.Builder, synchronous queries execute directly on the calling thread (blocking it until complete), as Room performs the underlying SQLite operations synchronously.[2][6][5] This is discouraged for production due to ANR risk but allowed for tests or trivial queries.[7][8] Recent sources (2025) confirm suspend functions and.allowMainThreadQueries remain supported in Room 2.x (current stable as of 2026), with Room 3.0 emphasizing coroutines but in maintenance for 2.x.[9][8][10] One 2024 SO comment claims allowMainThreadQueries "does not exist anymore," but official docs contradict this.[5]
Citations:
- 1: https://stackoverflow.com/questions/57917333/accessing-room-db-on-main-thread
- 2: https://stackoverflow.com/questions/44167111/android-room-simple-select-query-cannot-access-database-on-the-main-thread
- 3: https://commonsware.com/Room/pages/chap-roomthreads-001.html
- 4: https://developer.android.com/training/data-storage/room/async-queries
- 5: https://developer.android.com/reference/android/arch/persistence/room/RoomDatabase.Builder
- 6: https://stackoverflow.com/questions/55841816/is-it-compulsory-to-write-queries-in-room-off-the-main-thread-in-android
- 7: https://stackoverflow.com/questions/52999851/why-is-recommended-not-to-use-allowmainthreadqueries-for-android-room
- 8: https://proandroiddev.com/the-hidden-dangers-of-room-database-performance-and-how-to-fix-them-ac93830885bd
- 9: https://stackoverflow.com/questions/79004734/android-room-synchronous-query
- 10: https://developer.android.com/blog/posts/modernizing-the-room
이 메서드는 DAO 패턴을 따르지 않고 있습니다.
getAllProducts()가 동기적으로 List<CatalogEntity>를 반환하는 것이 문제입니다. 현재 코드에서 이 메서드는 사용되지 않고 있지만, 만약 호출된다면 다음과 같은 일이 발생할 수 있습니다:
- Room은 메인 스레드에서 동기 쿼리를 실행하는 것을 기본적으로 허용하지 않으며, 런타임에
IllegalStateException을 발생시킵니다 - 같은 파일의 다른 쿼리 메서드들(
getCartItemsWithProduct(),getRecentProductsWithDetail())은Flow를 반환하고 있습니다
같은 DAO 인터페이스에서 다른 쿼리 메서드들은 어떤 반환 타입을 사용하고 있는지 다시 확인해보세요. 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/dao/ShoppingDao.kt` around lines
21 - 22, The getAllProducts() DAO method currently returns a synchronous
List<CatalogEntity>, which can cause Room to throw on the main thread and is
inconsistent with other DAO methods; change its signature to return
kotlinx.coroutines.flow.Flow<List<CatalogEntity>> (i.e., fun getAllProducts():
Flow<List<CatalogEntity>>) so it uses Room's recommended asynchronous reactive
pattern consistent with getCartItemsWithProduct() and
getRecentProductsWithDetail(), and ensure the Flow import is added.
There was a problem hiding this comment.
코드래빗이 남겨준 것처럼 ShoppingDao 안에는 suspend인 함수와 그렇지 않은 함수들이 혼재하고 있습니다.
둘은 어떤 차이가 있는 것일까요?
| return this.copy(amount = newAmount) | ||
| } | ||
|
|
||
| fun addQuantity(requestedAmount: Int) = copy(amount = amount + requestedAmount) |
There was a problem hiding this comment.
addQuantity에서 음수 입력을 어떻게 처리할 계획인가요?
현재 addQuantity(requestedAmount: Int) 메서드는 전달받은 값에 대한 검증이 없습니다.
생각해볼 문제들:
- 만약
addQuantity(-5)를 호출하면 어떻게 되나요? amount가 음수가 되는 것을 방지하는 방법은 무엇일까요?decreaseQuantity와addQuantity의 입력 검증 전략이 일관성 있나요?
Line 14의 require 검사와 Line 18의 coerceAtLeast(0) 처리를 참고해서, addQuantity에서도 적절한 방어 로직을 고민해보세요.
🤖 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/CartProduct.kt` at line 22, The
addQuantity method in CartProduct currently lacks validation and will allow
negative requestedAmount (or reduce amount if negative); update
CartProduct.addQuantity(requestedAmount: Int) to validate requestedAmount (e.g.,
require(requestedAmount >= 0) with a clear message) or, if negative inputs
should be tolerated, normalize the result using amount +
requestedAmount.coerceAtLeast(-amount) or finalAmount.coerceAtLeast(0) to
prevent negative totals; ensure the chosen approach is consistent with
decreaseQuantity and the existing require on line 14 and the coerceAtLeast(0)
logic on line 18 so both methods enforce the same invariants.
Comment on lines
+31
to
+43
| fun decreaseQuantityOfCartProduct(productId: UUID, minusAmount: Int = 1): CartProducts { | ||
| if (findSameProduct(productId) == null) return this | ||
|
|
||
| val updatedProducts = products.map { cartProduct -> | ||
| if (cartProduct.product.productId == productId) { | ||
| cartProduct.decreaseQuantity(minusAmount) | ||
| } else { | ||
| cartProduct | ||
| } | ||
| } | ||
|
|
||
| return CartProducts(updatedProducts) | ||
| } |
There was a problem hiding this comment.
감소 후 수량 0인 항목이 장바구니에 남을 수 있습니다.
Line 34-42는 수량만 줄이고 항목 자체는 유지합니다. 그 결과 amount == 0 항목이 목록/카운트(uniqueItemCount)에 남아 UI와 상태가 어긋날 수 있습니다. 감소 결과 반영 시 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/CartProducts.kt` around lines
31 - 43, decreaseQuantityOfCartProduct currently only reduces quantity and keeps
items with amount==0; after calling CartProduct.decreaseQuantity inside
decreaseQuantityOfCartProduct you should remove any cart products whose quantity
became 0 so uniqueItemCount/UI stay correct. Locate
decreaseQuantityOfCartProduct (and use findSameProduct and
CartProduct.decreaseQuantity to find behavior), produce the updatedProducts by
mapping then filtering out products with amount <= 0 (or otherwise
skipping/omitting null/zero results), and return new
CartProducts(updatedProducts) so zero-quantity items are not retained.
Comment on lines
+32
to
37
| val app = application as ShoppingApplication | ||
| val productRepository = app.productRepository | ||
| val product = productRepository.getProductById(productId) ?: return finish() | ||
| val cartRepository = app.cartRepository | ||
| val recentProductRepository = app.recentProductRepository | ||
| val toast = Toast.makeText(this, "장바구니에 담았습니다", Toast.LENGTH_SHORT) |
There was a problem hiding this comment.
getProductById 호출이 메인 스레드를 블로킹할 수 있습니다.
HttpProductRepository.getProductById 구현을 보면 내부에서 runBlocking으로 네트워크 요청을 동기 실행하고 있어요. onCreate(메인 스레드)에서 호출되면 네트워크 지연 동안 UI 스레드가 멈추거나 ANR이 발생할 수 있습니다.
- 이 화면을 진입할 때 굳이 네트워크를 다시 거쳐 상품을 다시 조회해야 할까요? 인텐트로 필요한 정보만 전달하거나, 캐시/로컬 DB에서 조회하도록 분리하는 방법은 어떨까요?
- 정 네트워크 조회가 필요하다면, 함수 시그니처를
suspend로 바꾸고lifecycleScope.launch { ... }안에서 호출하는 흐름을 고려해 보세요.
루트 원인은 HttpProductRepository.kt의 getProductById에서 함께 다루겠습니다.
🤖 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/ProductDetailActivity.kt` around lines
32 - 37, ProductDetailActivity calls productRepository.getProductById on the
main thread, which blocks because HttpProductRepository uses runBlocking; fix by
avoiding synchronous network calls on onCreate: either (A) pass required Product
data via the Intent from the previous Activity or read from a local cache/DB
exposed by ShoppingApplication.productRepository so no network call is needed in
ProductDetailActivity, or (B) make getProductById a suspend function in the
repository and update call sites to invoke it inside lifecycleScope.launch
within ProductDetailActivity (use lifecycleScope.launch { val product =
productRepository.getProductById(productId); if (product == null) finish() else
bindUI(product) }) so the network request does not block the UI thread; update
ShoppingApplication and all repository implementations accordingly (referenced
symbols: ProductDetailActivity, ShoppingApplication, productRepository,
getProductById, lifecycleScope.launch).
There was a problem hiding this comment.
우선 runBlocking으로 구현한 특별한 이유가 있는지 궁금한데요.
코드래빗이 남겨준 코멘트와 이전 리뷰에서 남겨드렸던 부분을 하나씩 되짚어보시면 아마 어떻게 해결해야 할지 감이 잡힐 것 같습니다.
만약 어렵다면 언제든 연락주시면 되겠습니다 😄
Comment on lines
+43
to
65
| fun onIncreaseProduct(id: UUID) { | ||
| val cartProduct = cart.cartProducts.findSameProduct(id) ?: return | ||
| coroutineScope.launch { | ||
| cartRepository.addProduct(cartProduct.product, 1) | ||
| } | ||
| } | ||
|
|
||
| val maxValidPage = if (cart.size() == 0) 0 else (cart.size() - 1) / ONE_PAGE_ITEM_COUNT | ||
| if (currentPage > maxValidPage) currentPage = maxValidPage | ||
| fun onDecreaseProduct(id: UUID) { | ||
| val cartProduct = cart.cartProducts.findSameProduct(id) ?: return | ||
| coroutineScope.launch { | ||
| if (cartProduct.amount > 1) { | ||
| cartRepository.decreaseProduct(id, 1) | ||
| } else { | ||
| cartRepository.removeProduct(id) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun onDeleteProduct(id: UUID) { | ||
| coroutineScope.launch { | ||
| cartRepository.removeProduct(id) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: CartRepository 구현에서 동시성 처리 방식 확인
rg -nP --type=kt -A10 'fun addProduct|fun decreaseProduct|fun removeProduct' -g '**/CartRepository.kt' -g '**/RoomCartRepository.kt'Repository: woowacourse/android-shopping-cart
Length of output: 104
🏁 Script executed:
find . -type f -name "*CartRepository*" | head -20Repository: woowacourse/android-shopping-cart
Length of output: 344
🏁 Script executed:
cat -n ./app/src/main/java/woowacourse/shopping/repository/cartRepository/CartRepository.ktRepository: woowacourse/android-shopping-cart
Length of output: 643
🏁 Script executed:
cat -n ./app/src/main/java/woowacourse/shopping/repository/cartRepository/RoomCartRepository.ktRepository: woowacourse/android-shopping-cart
Length of output: 2641
🏁 Script executed:
cat -n ./app/src/main/java/woowacourse/shopping/repository/cartRepository/InMemoryCartRepository.ktRepository: woowacourse/android-shopping-cart
Length of output: 1345
🏁 Script executed:
# Let me check if there are any Mutex or synchronization mechanisms in use
rg -n "Mutex|withLock|synchronized" --type=kt -g '**/CartRepository*'Repository: woowacourse/android-shopping-cart
Length of output: 104
🏁 Script executed:
rg -n "Mutex|withLock|synchronized" app/src/main/java/woowacourse/shopping/repository/cartRepository/Repository: woowacourse/android-shopping-cart
Length of output: 59
🏁 Script executed:
find . -type f -name "ShoppingDao*"Repository: woowacourse/android-shopping-cart
Length of output: 142
🏁 Script executed:
cat -n ./app/src/main/java/woowacourse/shopping/data/dao/ShoppingDao.ktRepository: woowacourse/android-shopping-cart
Length of output: 2103
🏁 Script executed:
# Check if Room database has any transaction/synchronization at the database level
# Look for Database configuration
find . -type f -name "*Database.kt" -o -name "*RoomDatabase*"Repository: woowacourse/android-shopping-cart
Length of output: 146
🏁 Script executed:
cat -n ./app/src/main/java/woowacourse/shopping/data/db/ShoppingDatabase.ktRepository: woowacourse/android-shopping-cart
Length of output: 815
동시성 문제를 해결하세요.
RoomCartRepository의 각 메서드(addProduct, decreaseProduct)가 여러 개의 데이터베이스 작업을 순차적으로 수행하고 있습니다:
addProduct: 상품 삽입 → 수량 조회 → 수량 업데이트/삽입decreaseProduct: 수량 조회 → 수량 업데이트/삭제
이 작업들이 별도의 트랜잭션으로 실행되므로, 여러 코루틴이 동시에 실행되면 읽은 값과 쓴 값 사이의 시간차로 인해 예상치 못한 결과가 발생할 수 있습니다. 예를 들어, 두 코루틴이 동시에 같은 수량을 읽고 각각 계산한 후 덮어쓰면 한 쪽의 변경이 손실됩니다.
InMemoryCartRepository도 MutableStateFlow.value 할당이 동시에 일어나면 업데이트가 손실될 수 있습니다.
다음 방법들을 고려해보세요:
- Room의
@Transaction어노테이션이 여러 작업을 원자적으로 처리하는 방식 - Kotlin의
Mutex를 이용한 동시 접근 제어
먼저 테스트로 이 문제를 재현해보고, 그 후 어떤 접근 방식이 적절한지 판단해보세요.
🤖 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/stateholder/CartStateHolder.kt`
around lines 43 - 65, The handlers
onIncreaseProduct/onDecreaseProduct/onDeleteProduct call
cartRepository.addProduct/decreaseProduct/removeProduct which perform multiple
DB reads/writes and can race; fix by making repository operations atomic and
serialized: either mark the multi-step Room DAO methods or repository methods
with `@Transaction` so Room performs the read/update/insert as one transaction, or
add a coroutine Mutex inside RoomCartRepository and InMemoryCartRepository to
guard the multi-step methods (acquire lock around
addProduct/decreaseProduct/removeProduct and update MutableStateFlow.value under
the same lock); add a concurrent test that launches multiple coroutines calling
onIncreaseProduct/onDecreaseProduct to reproduce the issue and verify the chosen
fix prevents lost updates.
There was a problem hiding this comment.
Room의 @Transaction은 무엇이고 어떤 동작을 기대할 수 있는지 확인해보면 좋겠네요.
Comment on lines
76
to
80
| private fun loadProducts() { | ||
| val products = runBlocking { | ||
| mockingCatalog.loadProducts(recentItemIndex, MAX_PRODUCT).await() | ||
| productRepository.getProducts(recentItemIndex, MAX_PRODUCT) | ||
| } | ||
| _catalog.value += products |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/src/main/java/woowacourse/shopping/ui/stateholder/CatalogScreenStateHolder.kt | head -100Repository: woowacourse/android-shopping-cart
Length of output: 3441
🏁 Script executed:
find . -name "ProductRepository*" -type f | head -20Repository: woowacourse/android-shopping-cart
Length of output: 168
🏁 Script executed:
cat -n ./app/src/main/java/woowacourse/shopping/repository/productRepository/ProductRepository.ktRepository: woowacourse/android-shopping-cart
Length of output: 409
🏁 Script executed:
find . -name "*ProductRepository*.kt" -type f | xargs grep -l "suspend fun getProducts"Repository: woowacourse/android-shopping-cart
Length of output: 361
🏁 Script executed:
cat -n ./app/src/main/java/woowacourse/shopping/repository/productRepository/HttpProductRepository.ktRepository: woowacourse/android-shopping-cart
Length of output: 1955
메인 스레드 경로에서 runBlocking으로 제품 로딩을 차단하고 있습니다.
loadProducts()는 생성 시점(line 39)과 더보기 클릭(line 73)에서 동기 대기를 거는데, HTTP 응답이 늦을 때 초기 진입이나 pagination 클릭이 그대로 멈춥니다. ProductRepository.getProducts()는 suspend 함수로 설계되어 있고, 구현체인 HttpProductRepository도 withContext(Dispatchers.IO)로 올바르게 비동기 처리하고 있습니다.
여기서 "반드시 지금-여기서 결과가 필요한가?"를 다시 점검해 보세요. 이미 클래스에 coroutineScope가 있고 다른 비동기 작업(line 55, 62)에서 활용 중인데, 이를 어떻게 활용할 수 있을지 생각해 보시면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/woowacourse/shopping/ui/stateholder/CatalogScreenStateHolder.kt`
around lines 76 - 80, loadProducts() blocks the main thread via runBlocking;
instead, call the suspend productRepository.getProducts(...) from the existing
coroutineScope by launching a coroutine (e.g., coroutineScope.launch { ... })
and update _catalog.value with the result inside that coroutine. Replace the
runBlocking block in loadProducts() with a coroutineScope.launch that invokes
productRepository.getProducts(recentItemIndex, MAX_PRODUCT), appends to
_catalog, and add basic error handling (try/catch) to surface/log failures; keep
using the repository's internal Dispatchers.IO so the call remains off the main
thread.
Comment on lines
+89
to
95
| fun retainCatalogScreenStateHolder( | ||
| repository: ProductRepository, | ||
| ): CatalogScreenStateHolder { | ||
| val scope = rememberCoroutineScope() | ||
| return retain(Unit) { | ||
| CatalogScreenStateHolder(repository, coroutineScope = scope) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(CatalogScreenStateHolder|retain)" | head -20Repository: woowacourse/android-shopping-cart
Length of output: 159
🏁 Script executed:
cat -n app/src/main/java/woowacourse/shopping/ui/stateholder/CatalogScreenStateHolder.ktRepository: woowacourse/android-shopping-cart
Length of output: 3441
🏁 Script executed:
fd -t f "\.kt$" | xargs rg -l "fun retain" | head -10Repository: woowacourse/android-shopping-cart
Length of output: 159
🏁 Script executed:
rg "retain\(" -A 5 -B 5 app/src/main/java/ | head -50Repository: woowacourse/android-shopping-cart
Length of output: 1072
🏁 Script executed:
rg "retainCatalogScreenStateHolder" -r -A 10 -B 2Repository: woowacourse/android-shopping-cart
Length of output: 120
🏁 Script executed:
fd -t f "\.kt$" | xargs rg -l "cartFlow\|onIncrease\|onDecrease" | head -10Repository: woowacourse/android-shopping-cart
Length of output: 59
🏁 Script executed:
find . -name "*.kt" -path "*/test/*" | xargs rg -l "CatalogScreenStateHolder\|retainCatalogScreenStateHolder" 2>/dev/nullRepository: woowacourse/android-shopping-cart
Length of output: 61
🏁 Script executed:
rg "rememberCoroutineScope|retain" app/src/main/java/ -B 3 -A 3 | head -60Repository: woowacourse/android-shopping-cart
Length of output: 2245
🏁 Script executed:
find . -name "*.kt" | xargs rg "class.*StateHolder" | head -20Repository: woowacourse/android-shopping-cart
Length of output: 291
🏁 Script executed:
cat -n app/src/main/java/woowacourse/shopping/ui/stateholder/CartStateHolder.kt | head -100Repository: woowacourse/android-shopping-cart
Length of output: 3466
🏁 Script executed:
rg "DisposableEffect|LaunchedEffect|CoroutineScope" app/src/main/java/ | head -20Repository: woowacourse/android-shopping-cart
Length of output: 1521
🏁 Script executed:
cat -n app/src/main/java/woowacourse/shopping/MainActivity.kt | head -150Repository: woowacourse/android-shopping-cart
Length of output: 4211
🏁 Script executed:
rg "retain.*rememberCoroutineScope|rememberCoroutineScope.*retain" app/src/main/java/ -B 2 -A 5Repository: woowacourse/android-shopping-cart
Length of output: 59
retain 블록 내에서 재생성되지 않는 scope 참조 문제입니다.
Line 92의 rememberCoroutineScope()가 retain 블록 외부에서 호출되어 다음과 같은 생명주기 불일치가 발생합니다:
rememberCoroutineScope()는 composition에 묶여 있어 recomposition 시 취소됩니다retain(Unit)은CatalogScreenStateHolder인스턴스를 유지하므로 재생성되지 않습니다- 결과적으로 retain된 holder가 이미 취소된 scope를 참조하게 되어, 취소된 scope에서
cartFlow수집과onIncrease()/onDecrease()의 coroutine 작업이 무시됩니다
scope 관리 전략을 재검토하세요. holder의 생명주기를 연장하려면 scope의 생명주기도 함께 연장되어야 합니다.
🤖 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/stateholder/CatalogScreenStateHolder.kt`
around lines 89 - 95, The retainCatalogScreenStateHolder currently captures a
composition-tied scope from rememberCoroutineScope() outside retain, causing the
retained CatalogScreenStateHolder to reference a cancelled scope; fix by
removing rememberCoroutineScope() and instead create and manage a long-lived
scope inside the retain block (e.g., CoroutineScope(SupervisorJob() +
Dispatchers.Main)) when constructing CatalogScreenStateHolder (coroutineScope
parameter), and ensure you cancel that scope in retain's dispose/onRelease
callback so cartFlow collection and onIncrease()/onDecrease() coroutines run on
a scope with the same lifetime as the retained holder.
Comment on lines
+44
to
+50
| fun `겹치는 상품을 추가하면 전체 개수가 증가한다`() { | ||
| val cart = Cart(CartProducts(listOf(CartProduct(product = Product(imageUri = "image", name = "Samuel", price = 8000))))) | ||
| val newProduct = Product(imageUri = "image", name = "twohander", price = 10000) | ||
| val newCart = cart.addProduct(newProduct, 5) | ||
|
|
||
| assertEquals(6, newCart.getTotalQuantity()) | ||
| } |
There was a problem hiding this comment.
“중복 상품 증가” 테스트 시나리오가 현재 의도와 다릅니다.
Line 45의 기존 상품과 Line 46의 newProduct는 기본 생성 시 productId가 달라 실제로는 “중복 병합”이 아니라 “새 항목 추가” 검증이 됩니다. 이 상태에선 병합 로직 회귀를 놓칠 수 있어요.
🤖 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/CartTest.kt` around lines 44 -
50, The test "겹치는 상품을 추가하면 전체 개수가 증가한다" currently creates two Products with
different productIds so it validates adding a new item instead of merging;
update the test to use the same product identity when calling Cart.addProduct so
merge logic is exercised — e.g., construct the original CartProduct and the
newProduct with the same productId (or reuse the same Product instance) before
calling cart.addProduct(...), then assert via Cart.getTotalQuantity() that
quantities merged as expected; refer to Cart, CartProducts, CartProduct,
Product, addProduct and getTotalQuantity when making the change.
Redish03
commented
May 11, 2026
Author
There was a problem hiding this comment.
savedInsatanceState도 똑같이 Bundle에 저장하는 것으로 알고 있습니다.
우선적으로 Int 형은 8바이트, Uri는 제가 쓰던 url을 넣었을 때 200바이트, UUID는 16바이트, 이름은 약 15자 정도로 가정하면 30바이트 정도로 생각했습니다. 그러면 대략 250-300이라고 생각하고, 1MB가 1,048,576바이트 이므로 나누기를 해보면 대략 4000개 정도 나오네요! 권장 용량이 50kb로 알고 있는데, 나머지 부분이 거의 프로세스간 통신을 한다고 가정을 한다면 절반인 2000개가 될 것 같습니다. URL의 길이에 따라서 또 달라질 수 있을 것 같네요.. 🤔
There was a problem hiding this comment.
맞습니다. 사무엘이 이야기한대로 URL에 따라서도 달라질거에요.
1MB라는 한계에 우리는 얼만큼의 데이터를 넣을 수 있는지 직접 가늠해보는 것도 좋은 경험일거에요.
Comment on lines
118
to
124
| Column( | ||
| modifier = | ||
| modifier | ||
| .fillMaxSize() | ||
| .verticalScroll( | ||
| state = rememberScrollState(), | ||
| ), |
Author
There was a problem hiding this comment.
이제 확인했는데, LazyColumn 대신 일반 Column을 사용하신 특별한 이유가 있나요?
해당 부분에는 페이지네이션이 적용 되어 lazyColumn 으로 만드는 것 보다 Scrollable로 넣는게 더 효율이 좋을 것 같다고 생각했습니다!
There was a problem hiding this comment.
어떤 이유에서 효율이 좋다고 생각하였는지도 공유해주실 수 있을까요? 😄
Author
There was a problem hiding this comment.
패키징 분리에 대해서 고민을 좀 많이 해봤었는데요, 다른 크루들은 대부분 크게
/domain
/data
/ui
- stateHolder
- viewModel
- component
이런식으로 나누는 것 같았습니다.
저는 사실 Activity 별로 나누는게 좋을 것 같다고 생각했는데, 다른 크루의 패키지 구조가 조금 어렵게 느껴지기도 하네요.. 🤔 그래도 패키지별로 의존하는 방향성이 달라진다는 장점도 있는 것 같습니다.
There was a problem hiding this comment.
패키징 분리에 정해진 답은 없습니다.
여러 코드들을 읽어보고 사무엘만의 기준을 한 번 만들어보시면 좋을 것 같아요 👍
Author
There was a problem hiding this comment.
해당하는 객체는 안드로이드에 의존하다고 생각해 Domain이나 Data 계층에 넣지는 않았는데, 그렇다고 컴포넌트나 화면에 보이는 UI 로직인가...? 라는 생각도 들었습니다
어떻게 보면 기기 자체에 되게 의존한다고 생각해 UI인가? 사용자가 네트워크 환경에서 벗어나거나 연결에 대해서 생각을 해야해서 UI 인가? 라고 생각도 들어서 UI 같기도 하다는 생각이 들기도 하는데, 사용자랑 직접 상호작용을 하는 역할은 또 아니라는 생각이 들었습니다
혹시 어떤 패키지에 위치하실것 같은지 궁금합니다,,
There was a problem hiding this comment.
그 전에 NetworkMonitor가 현재 단계에서 필요한 것일까용?.?
krrong
requested changes
May 12, 2026
krrong
left a comment
krrong
left a comment
There was a problem hiding this comment.
안녕하세요 사무엘,
이번 미션 고생 많으셨습니다. 🙌
이번 미션에서 ViewModel도 중요한 학습 목표중 하나로 알고 있는데요.
아직 모든 화면에 적용되지는 않은 것 같은데 시간이 걸리더라도 천천히 리팩터링을 해보셨으면 좋겠습니다.
고민해볼만한 부분들에 코멘트를 남겨두었으니, 코드래빗의 코멘트와 함께 확인해주세요 🙇
어려운 부분이 있다면 언제든 연락주시면 됩니다! 💪
Comment on lines
+21
to
+22
| @Query("SELECT * FROM products") | ||
| fun getAllProducts(): List<CatalogEntity> |
There was a problem hiding this comment.
코드래빗이 남겨준 것처럼 ShoppingDao 안에는 suspend인 함수와 그렇지 않은 함수들이 혼재하고 있습니다.
둘은 어떤 차이가 있는 것일까요?
| return this.copy(amount = newAmount) | ||
| } | ||
|
|
||
| fun addQuantity(requestedAmount: Int) = copy(amount = amount + requestedAmount) |
Comment on lines
+32
to
37
| val app = application as ShoppingApplication | ||
| val productRepository = app.productRepository | ||
| val product = productRepository.getProductById(productId) ?: return finish() | ||
| val cartRepository = app.cartRepository | ||
| val recentProductRepository = app.recentProductRepository | ||
| val toast = Toast.makeText(this, "장바구니에 담았습니다", Toast.LENGTH_SHORT) |
There was a problem hiding this comment.
우선 runBlocking으로 구현한 특별한 이유가 있는지 궁금한데요.
코드래빗이 남겨준 코멘트와 이전 리뷰에서 남겨드렸던 부분을 하나씩 되짚어보시면 아마 어떻게 해결해야 할지 감이 잡힐 것 같습니다.
만약 어렵다면 언제든 연락주시면 되겠습니다 😄
Comment on lines
+43
to
65
| fun onIncreaseProduct(id: UUID) { | ||
| val cartProduct = cart.cartProducts.findSameProduct(id) ?: return | ||
| coroutineScope.launch { | ||
| cartRepository.addProduct(cartProduct.product, 1) | ||
| } | ||
| } | ||
|
|
||
| val maxValidPage = if (cart.size() == 0) 0 else (cart.size() - 1) / ONE_PAGE_ITEM_COUNT | ||
| if (currentPage > maxValidPage) currentPage = maxValidPage | ||
| fun onDecreaseProduct(id: UUID) { | ||
| val cartProduct = cart.cartProducts.findSameProduct(id) ?: return | ||
| coroutineScope.launch { | ||
| if (cartProduct.amount > 1) { | ||
| cartRepository.decreaseProduct(id, 1) | ||
| } else { | ||
| cartRepository.removeProduct(id) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun onDeleteProduct(id: UUID) { | ||
| coroutineScope.launch { | ||
| cartRepository.removeProduct(id) | ||
| } | ||
| } |
There was a problem hiding this comment.
Room의 @Transaction은 무엇이고 어떤 동작을 기대할 수 있는지 확인해보면 좋겠네요.
Comment on lines
118
to
124
| Column( | ||
| modifier = | ||
| modifier | ||
| .fillMaxSize() | ||
| .verticalScroll( | ||
| state = rememberScrollState(), | ||
| ), |
There was a problem hiding this comment.
어떤 이유에서 효율이 좋다고 생각하였는지도 공유해주실 수 있을까요? 😄
Comment on lines
+39
to
+41
| override fun getProductById(id: UUID): Product? = runBlocking { | ||
| getProducts(0, 20).find { it.productId == id } | ||
| } |
There was a problem hiding this comment.
21번째 이후 상품을 선택하여 상세 페이지로 이동해보면 문제가 발생하고 있을거에요.
우리는 왜 getProductById 함수에 runBlocking을 추가해야만 했을까요?
어떻게 하면 다른 방식으로 해결해볼 수 있을까요?
| private val json = Json { ignoreUnknownKeys = true } | ||
|
|
||
| override suspend fun getProducts(page: Int, pageSize: Int): List<Product> = | ||
| withContext(Dispatchers.IO) { |
There was a problem hiding this comment.
withContext를 사용하면 어떤 동작을 하게 될까요?
Dispatcher를 IO로 바꿔주는 이유는 무엇인가요?
There was a problem hiding this comment.
맞습니다. 사무엘이 이야기한대로 URL에 따라서도 달라질거에요.
1MB라는 한계에 우리는 얼만큼의 데이터를 넣을 수 있는지 직접 가늠해보는 것도 좋은 경험일거에요.
Comment on lines
+8
to
+10
| interface CartRepository { | ||
| val cart: Cart | ||
| val cartFlow: StateFlow<Cart> |
There was a problem hiding this comment.
StateHolder → ViewModel 로 리팩터링을 한 번 해보시면서 이러한 값들은 어디에 저장되어야 할지 고민해보시면 좋을 것 같아요.
Comment on lines
+18
to
+19
| @Insert(onConflict = OnConflictStrategy.IGNORE) | ||
| suspend fun insertProduct(products: List<CatalogEntity>) |
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에 기능 목록을 정리하고 명확히 기술하였는가?코드 리뷰 커뮤니케이션
안녕하세요 크롱, 3, 4단계 제출합니다
솔직하게 말하자면 시간 상 모든 것들을 반영하고 기능도 만들기엔 시간이 좀 촉박하다고 느껴져서 AI를 좀 활용을 했습니다 하하
그냥 아무 생각 없이 생성해서 하지는 않았는데, OkHTTP 관련된건 좀 많이 어려웠던 것 같습니다 😢
이전 리뷰 반영에 대해서는 디엠으로 연락드리도록 하겠습니다!
스크린샷