[3단계 - Navigation & Flow | 4단계 - 주문] 모스 미션 제출합니다.#177
Open
katie0109 wants to merge 39 commits into
Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough이 PR은 Android 쇼핑 앱의 주문 결제 기능을 구현하고 Navigation 아키텍처를 재설계하는 대규모 변경입니다. 쿠폰 도메인 모델부터 데이터 계층, 결제 UI 화면까지 일관된 결제 시스템을 추가하고, Activity 기반의 화면 전환을 Jetpack Compose Navigation으로 통합합니다. SharedFlow 기반의 UI 이벤트 처리를 도입하여 ViewModel에서 화면 간 네비게이션을 제어하며, 결제 알림 및 리마인더 시스템을 구현합니다. 또한 MockServer 및 기존 Activity 클래스(ProductListActivity, ProductDetailActivity, CartActivity)를 제거하고, 카트 저장소에 변경 이벤트 스트림을 추가하여 화면 간 상태 동기화를 강화합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (15)
app/src/main/java/woowacourse/shopping/domain/model/coupon/CouponTypes.kt (1)
14-20: ⚡ Quick win쿠폰 코드 매핑 전에 입력 정규화를 한 단계 추가하는 게 안전합니다.
Line 14-20은 대문자 완전일치만 허용해서,
"fixed5000"또는 공백 포함 입력이UNKNOWN으로 분류될 수 있습니다.trim + uppercase같은 정규화 단계 분리를 고려해 보시겠어요?As per coding guidelines, "Break problems into smaller, manageable steps to help think through them logically."
🤖 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/model/coupon/CouponTypes.kt` around lines 14 - 20, The fromCode function in CouponTypes.kt currently matches codes case-sensitively and will treat " fixed5000 " or "fixed5000" as UNKNOWN; normalize the input first by applying trim() and uppercase (e.g., val normalized = code.trim().uppercase()) and then use normalized in the when expression to map to FIXED5000, BOGO, FREESHIPPING, MIRACLESALE, or UNKNOWN; update any tests or callers if they relied on exact-case behavior.app/src/main/java/woowacourse/shopping/ui/payment/PaymentUiState.kt (2)
15-15: ⚡ Quick win배송비가 항상 3000원인가요?
shippingFee가 기본값3000으로 하드코딩되어 있습니다.생각해 볼 점:
- 배송비가 주문 금액, 지역, 프로모션 등에 따라 달라질 수 있나요?
- 만약 변동 가능하다면, 이 값을 어디서 계산/주입하는 것이 적절할까요?
- 상수로 관리하는 것이 낫다면,
ShoppingConstants같은 곳으로 추출하는 건 어떨까요?현재는 UI State에 비즈니스 로직(배송비 정책)이 포함된 형태입니다.
🤖 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/payment/PaymentUiState.kt` at line 15, PaymentUiState currently hardcodes shippingFee = 3000 in the data class; remove the business-logic default and instead accept shippingFee as a provided value (no hardcoded default) so the UI state is populated by the domain/service layer that calculates fees, or if it truly is a constant move the value to a dedicated constant (e.g., ShoppingConstants.SHIPPING_FEE) and reference that constant from PaymentUiState; update any creators/factories that instantiate PaymentUiState (tests, viewmodels) to pass the correct shippingFee from the order calculation or constant.
9-17: 💤 Low valueSuccess 상태의 기본값들이 의미가 있을까요?
Success데이터 클래스에서subtotal,couponDiscount,totalPrice가 모두0으로 기본값이 설정되어 있습니다.질문:
- 이 값들은 계산되어야 하는 값들인가요, 아니면 초기 상태로 0이 의미가 있나요?
Success상태가 생성될 때 이미 계산된 값들로 채워져야 하는 건 아닐까요?- 기본값을 제공하는 것이 실수로 계산되지 않은 상태를 허용하게 만들 위험은 없을까요?
생각해 보세요: UI 상태가 항상 유효한 값을 가지도록 강제하는 것과, 기본값으로 유연성을 제공하는 것 사이의 트레이드오프는 무엇인가요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/payment/PaymentUiState.kt` around lines 9 - 17, The Success data class currently supplies default zeros for subtotal, couponDiscount, and totalPrice which can mask missing calculations; remove those default values (make subtotal, couponDiscount, totalPrice non-optional constructor parameters) or provide a single factory method (e.g., a Success.Companion.createFrom(cartItems, availableCoupons, selectedCoupon, shippingFee)) that computes and returns a fully populated Success instance so callers cannot accidentally construct an incomplete UI state; update call sites that instantiate Success to use the computed values or the factory.app/src/main/java/woowacourse/shopping/domain/exception/ProductException.kt (1)
20-22: 💤 Low value
data object에서object로 변경한 의도를 생각해 보셨나요?Sealed hierarchy 내의 다른 타입들(
ServerError,NetworkError,NotFound)은data class로 정의되어 있는데,EmptyBody만 일반object로 변경되었습니다.다음을 고려해 보세요:
data object와 일반object의 차이점은 무엇인가요? (힌트:toString(),equals(),hashCode()생성)EmptyBody가 프로퍼티를 갖지 않는다면,data object의 이점이 여전히 있을까요?- Sealed hierarchy 내에서 일관된 패턴을 유지하는 것이 중요할까요?
관련 문서: Kotlin data objects
🤖 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/exception/ProductException.kt` around lines 20 - 22, EmptyBody is declared as a plain object while other sealed ProductException subtypes (ServerError, NetworkError, NotFound) are data classes; change EmptyBody to a data object (i.e., make it a "data object EmptyBody : ProductException(...)") so it benefits from generated toString/equals/hashCode and maintains a consistent sealed-hierarchy pattern with the other types.app/src/main/java/woowacourse/shopping/constant/ShoppingColor.kt (1)
9-14: ⚖️ Poor tradeoff앱의 테마 시스템에 대해 고려해 보셨나요?
6개의 결제 관련 색상 상수가 하드코딩된 값으로 추가되었습니다. 기존 코드도 같은 패턴을 따르고 있긴 하지만, 생각해 볼 점이 있습니다:
질문:
- 다크 모드를 지원할 계획이 있나요?
- Material3의
ColorScheme를 사용하면 어떤 이점이 있을까요?- 색상 값이 여러 곳에서 사용될 때 중앙 집중식 테마 관리의 장점은 무엇일까요?
현재 구현은 기존 코드베이스와 일관성이 있지만, 향후 유지보수성을 위해 테마 시스템 도입을 고려해 볼 가치가 있습니다.
🤖 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/constant/ShoppingColor.kt` around lines 9 - 14, The six hardcoded ARGB color constants in ShoppingColor.kt (PAYMENT_BUTTON_COLOR, PAYMENT_DIVIDER_COLOR, PAYMENT_CARD_BORDER_COLOR, PAYMENT_SUMMARY_BACKGROUND_COLOR, PAYMENT_DESCRIPTION_GRAY, PAYMENT_TITLE_GRAY) should be moved out of raw ints and integrated into the app theme: create named color tokens in the central theme (light/dark variants) and expose them via Material3 ColorScheme or your Theme object, then replace usages of these constants to read from the ColorScheme (or Theme.colors) so dark mode and centralized overrides work; update any callers that reference these constants to use the themed color tokens instead.app/src/main/java/woowacourse/shopping/ui/util/PaymentReminderReceiver.kt (1)
9-20: ⚡ Quick winBroadcastReceiver의 실행 환경을 고려해 보셨나요?
BroadcastReceiver의onReceive는 메인 스레드에서 실행되며 약 10초의 제한 시간이 있습니다. 현재 구현에서 고려해 볼 점들:
NotificationHelper.createNotificationChannel과showPaymentReminder호출이 예외를 발생시키면 어떻게 될까요?- 이 두 메서드가 메인 스레드에서 안전하게 실행될 수 있을 만큼 빠른지 확인해 보셨나요?
다음을 생각해 보세요:
- 알림 생성 중 예외가 발생할 수 있는 시나리오는 무엇일까요? (예: 권한 거부, 시스템 리소스 부족)
- 이러한 예외를 어떻게 처리하면 좋을까요?
🤖 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/util/PaymentReminderReceiver.kt` around lines 9 - 20, onReceive in PaymentReminderReceiver runs on the main thread with a short timeout, so wrap the notification work in a background task and add robust exception handling: call goAsync() inside PaymentReminderReceiver.onReceive to obtain a PendingResult, then offload NotificationHelper.createNotificationChannel and NotificationHelper.showPaymentReminder to a background thread/coroutine (or WorkManager) and call finish() on the PendingResult when done; surround the background work with try/catch to handle and log exceptions (e.g., permission issues, resource errors) and ensure NotificationSettings.isNotificationEnabled is checked before dispatching work; also validate the intent extra (PaymentReminderScheduler.EXTRA_SELECTED_ITEM_IDS) and handle null/empty safely.app/src/main/java/woowacourse/shopping/data/remote/dto/CouponResponseDto.kt (1)
10-10: 💤 Low value쿠폰 타입의 타입 안정성에 대해 생각해 보셨나요?
discountType이 현재String으로 정의되어 있습니다. PR 설명에서CouponMapper가 타입 정규화를 처리한다고 하셨는데요:질문:
String타입을 사용할 때의 장단점은 무엇인가요?- 잘못된 문자열 값이 들어올 경우 어디서 감지되나요? (DTO 레벨? Mapper 레벨? Domain 레벨?)
- 매핑 단계에서 변환한다고 하셨는데, 그렇다면 DTO가
String을 받는 현재 설계가 적절해 보이시나요?생각해 볼 점: DTO는 서버 응답을 있는 그대로 표현하고, 매퍼에서 도메인 타입으로 변환하는 접근이 합리적인지 검토해 보세요.
🤖 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/dto/CouponResponseDto.kt` at line 10, The DTO field discountType is currently a raw String which risks invalid values slipping past compile-time; update the design by either 1) making CouponResponseDto use a dedicated enum (e.g., DiscountType) to enforce type-safety at the DTO boundary, or 2) if the API can return arbitrary strings, keep String but add strict validation and clear error handling in CouponMapper (the mapping function that converts CouponResponseDto -> domain coupon) to translate/normalize strings into the domain DiscountType enum and surface mapping errors early; locate CouponResponseDto and the CouponMapper mapping logic and implement one of these approaches so invalid types are detected and handled at the mapper/domain boundary rather than silently propagated.app/src/main/java/woowacourse/shopping/ui/util/NotificationSettings.kt (1)
10-28: ⚡ Quick win반복되는 SharedPreferences 접근 코드를 개선할 수 있습니다.
각 함수에서
context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE)를 반복적으로 호출하고 있습니다.이 중복을 줄이는 방법에는 어떤 것들이 있을까요?
- private 프로퍼티로 SharedPreferences 인스턴스를 캐싱하는 방법
- 확장 함수를 활용하는 방법
- Context를 생성자로 받아 초기화 시점에 SharedPreferences를 받는 방법
각 방법의 장단점을 고려해보고, 코드의 가독성과 성능을 개선해보세요.
🤖 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/util/NotificationSettings.kt` around lines 10 - 28, Refactor repeated context.getSharedPreferences calls in NotificationSettings.kt by caching a SharedPreferences instance and use it across functions: create a private lazy or initialized property (e.g., private lateinit or private val sharedPrefs) that is set once (via an init method that accepts Context or by providing Context to an initialize(context) function) and then replace calls inside isNotificationEnabled, setNotificationEnabled, markReminderScheduled, and hasScheduledReminder to use that cached sharedPrefs; alternatively consider a Context extension that returns the prefs to keep call sites concise—ensure initialization happens before any getter/setter is used to avoid NPEs.app/src/main/java/woowacourse/shopping/ui/productList/ProductListUiState.kt (1)
7-7: 💤 Low value
data object에서object로 변경한 이유가 궁금합니다.
Loading이data object에서 일반object로 변경되었습니다.Kotlin 1.9부터 도입된
data object는 싱글톤 객체에equals(),hashCode(),toString()을 자동 생성해주는데, 이 케이스에서는 이러한 메서드들이 필요하지 않다고 판단하신 것인가요? 아니면 다른 이유가 있으신가요?기능적으로는 차이가 없지만, 일관성 측면에서 팀의 코딩 컨벤션이나 선호하는 스타일이 있다면 참고하시면 좋을 것 같습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productList/ProductListUiState.kt` at line 7, Change the singleton declaration back to a data object to keep consistency and retain generated equals/hashCode/toString: replace the current "object Loading : ProductListUiState" with "data object Loading : ProductListUiState" in the ProductListUiState declaration (refer to the ProductListUiState.Loading symbol) so it uses Kotlin 1.9's data object behavior.app/src/main/java/woowacourse/shopping/ui/common/SettingsScreen.kt (1)
36-38: ⚡ Quick win상태 관리 접근 방식을 검토해보세요.
NotificationSettings.isNotificationEnabled(context)를remember블록 안에서 호출하고 있습니다. 이 방식은 화면이 처음 Composition될 때의 값만 읽어오며, 다른 화면이나 외부에서 설정이 변경되더라도 이 화면은 갱신되지 않습니다.이것이 의도된 동작인가요? 만약 설정 값의 변경을 관찰해야 한다면 어떤 패턴을 사용할 수 있을까요? (힌트: StateFlow, LiveData 등의 옵저버블 패턴)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/common/SettingsScreen.kt` around lines 36 - 38, 현재 SettingsScreen의 notificationEnabled 변수가 remember { mutableStateOf(NotificationSettings.isNotificationEnabled(context)) }로 초기화되어 초기 Composition 시 값만 읽히므로 외부에서 설정이 변경되어도 화면이 갱신되지 않습니다; NotificationSettings.isNotificationEnabled(context) 호출을 remember 안에서 제거하고, NotificationSettings에 StateFlow<Boolean> 또는 LiveData를 추가(예: NotificationSettings.notificationEnabledFlow)하여 SettingsScreen에서 해당 Flow/LiveData를 수집해 UI 상태로 변환(collectAsState 또는 observeAsState 등)하도록 변경하거나, NotificationSettings에 콜백/리스너를 제공해 변경시 mutableStateOf를 업데이트하도록 구현해 변경을 관찰 가능하게 만드세요.app/src/test/kotlin/woowacourse/shopping/domain/model/coupon/CouponCalculatorTest.kt (1)
161-193: ⚖️ Poor tradeoff시간 제한 쿠폰에 대한 경계 조건 테스트 추가를 고려해 보세요.
MIRACLESALE_오전_적용테스트에서availableStartTime/availableEndTime이 설정되지 않은 쿠폰을 사용하고 있어, 실제로는 "시간 제한 없음" 케이스를 테스트하고 있습니다.다음과 같은 추가 테스트를 고려해 보세요:
- 시간 범위가 설정된 쿠폰의 범위 내/외 시간 테스트
- 자정을 넘는 시간 범위 (예: 22:00~06:00) 테스트
- 만료된 쿠폰 테스트
이러한 테스트들이
CouponCalculator의 시간 로직 검증에 도움이 됩니다.🤖 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/kotlin/woowacourse/shopping/domain/model/coupon/CouponCalculatorTest.kt` around lines 161 - 193, Add boundary/time-range tests for CouponCalculator.apply: currently MIRACLESALE_오전_적용 and PERCENT_시간제한_없으면_항상_적용 use coupons without availableStartTime/availableEndTime so they only cover "no time limit". Create additional unit tests that construct Coupon instances with availableStartTime and availableEndTime set and call CouponCalculator.apply(cart, coupon, now=...) to assert applied/discount for (1) times inside the range, (2) times just outside the range (boundary minutes), (3) an overnight range that crosses midnight (e.g., availableStartTime=22:00, availableEndTime=06:00) with now at 23:00 and 05:00, and (4) an expired coupon (expireAt before now) to assert not applied; reference the Coupon class fields availableStartTime/availableEndTime/expireAt and the CouponCalculator.apply(...) and existing test names to add these cases.app/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailViewModel.kt (1)
94-107: 💤 Low value
delay(1000)하드코딩과 동작 의도에 대해 생각해 보세요.장바구니에 담은 후 1초 지연 후 자동 네비게이션을 수행하고 있습니다. 몇 가지 고려할 점이 있습니다:
- 사용자가 스낵바를 보고 1초 내에 다른 액션을 수행하면 어떻게 될까요?
- 지연 시간을 상수로 추출하면 의도가 더 명확해집니다.
- 사용자가 화면을 빠르게 이탈하면
viewModelScope가 취소되어 이벤트가 방출되지 않을 수 있는데, 이것이 의도된 동작인지 확인해 보세요.스낵바에 "장바구니로 이동" 액션 버튼을 추가하는 방식도 UX 관점에서 고려해 볼 만합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailViewModel.kt` around lines 94 - 107, The addToCart() function currently uses a hardcoded delay(1000) after emitting _uiEvent ShowSnackbar which can cause unintended cancellation or poor UX; extract the timeout into a named constant (e.g., AUTO_NAV_DELAY_MS) and replace delay(1000) with that constant, and instead of auto-navigating unconditionally from viewModelScope, change the flow to either (A) expose a snackbar event with an action button that triggers _uiEvent.NavigateToCart when the user taps it, or (B) perform the delayed navigation using viewModelScope.launch { withContext(NonCancellable) { delay(AUTO_NAV_DELAY_MS); _uiEvent.emit(UiEvent.NavigateToCart) } } if you intend navigation to proceed even if the caller cancels; update references to _uiState, cartRepository, _uiEvent, and addToCart() accordingly so the intent is clear.app/src/main/java/woowacourse/shopping/ui/util/PaymentReminderScheduler.kt (1)
41-46: 💤 Low value
setAndAllowWhileIdle사용은 적절하지만, 알람 정확도에 대해 인지하고 있어야 합니다.
setAndAllowWhileIdle은 Doze 모드에서도 실행되지만, 시스템이 배터리 최적화를 위해 실행 시점을 조정할 수 있어 정확한 시간에 실행되지 않을 수 있습니다. 현재 결제 리마인더 용도로는 적절한 선택입니다.만약 정확한 타이밍이 필요하다면
setExactAndAllowWhileIdle과SCHEDULE_EXACT_ALARM권한이 필요하므로, 현재 접근 방식이 권한 요청 없이 동작하는 합리적인 트레이드오프입니다.🤖 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/util/PaymentReminderScheduler.kt` around lines 41 - 46, The current use of AlarmManager.setAndAllowWhileIdle in PaymentReminderScheduler (the alarmManager.setAndAllowWhileIdle call) is inexact and may be deferred by Doze; if you need guaranteed exact firing change the call to AlarmManager.setExactAndAllowWhileIdle and add handling to request/declare the SCHEDULE_EXACT_ALARM permission (and fall back to setAndAllowWhileIdle when the permission isn't granted). Update the scheduler logic around the existing alarmManager.cancel(...) / set... sequence to perform the permission check, request or gracefully degrade to the inexact setter, and keep the same PendingIntent usage (PaymentReminderScheduler / the pendingIntent creation) so behavior remains consistent.app/src/main/java/woowacourse/shopping/ui/productList/ProductListViewModel.kt (1)
64-70: 💤 Low value
addProduct후refreshCart()호출과cartEvents수신이 중복될 수 있습니다.
addProduct내에서refreshCart()를 직접 호출하고, 동시에cartEvents를 통해서도refreshCart()가 트리거됩니다. 이로 인해 장바구니 데이터를 두 번 가져올 수 있습니다.현재 동작에 문제는 없지만, 네트워크 호출 최적화가 필요하다면 둘 중 하나의 방식으로 통일하는 것을 고려해 보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/ui/productList/ProductListViewModel.kt` around lines 64 - 70, The addProduct method calls refreshCart() directly and cartEvents also triggers refreshCart(), causing duplicate fetches; remove the direct refreshCart() call inside addProduct (keep emitting _uiEvent.ShowSnackbar) and rely on the existing cartEvents flow to refresh the cart, or alternatively implement a debounce/guard if you prefer event-driven + immediate refresh; update the addProduct function (referencing addProduct, refreshCart, and cartEvents) to eliminate the redundant refresh invocation.app/src/main/java/woowacourse/shopping/domain/model/coupon/CouponCalculator.kt (1)
120-123: 💤 Low value퍼센트 할인 계산 시
toInt()사용에 대해 검토해 보세요.
(subtotal * rate).toInt()는 소수점 이하를 버림 처리합니다. 예를 들어 15,001원의 10% 할인은 1,500.1원이지만 1,500원으로 계산됩니다.비즈니스 요구사항에 따라
roundToInt()(반올림) 또는 현재 방식(버림)이 적절할 수 있습니다. 의도된 동작이라면 주석으로 명시하면 좋겠습니다.🤖 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/model/coupon/CouponCalculator.kt` around lines 120 - 123, The percent-discount calculation currently truncates fractions using (subtotal * rate).toInt(), losing cents; change this to use rounding by replacing that expression with (subtotal * rate).roundToInt() (and add the kotlin.math.roundToInt import), keep appliedDiscount = discount.coerceAtMost(subtotal) and the returned CouponApplyResult as-is; if truncation was intentional instead, add a clear inline comment above the rate/discount calculation in CouponCalculator.kt explaining that .toInt() (flooring) is deliberate.
🤖 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 @.gitignore:
- Line 42: 현재 .gitignore에 추가된 app/src/main/res/xml/network_security_config.xml
항목 때문에 빌드/CI 환경에서 네트워크 보안 리소스가 누락될 위험이 있으니 해당 파일을 .gitignore에서 제거하고 저장소에 포함시켜
버전관리를 복원하세요; 만약 network_security_config.xml 내부에 민감한 값(인증서, 도메인 키 등)이 있다면 파일 자체를
커밋하지 말고, 대신 network_security_config.xml에서 민감값을 분리하여 환경별 주입(빌드 변수, CI 시크릿, 또는 리소스
템플릿 + 변환 스텝)으로 대체하도록 리팩토링하세요.
In `@app/build.gradle.kts`:
- Around line 36-40: buildConfigField("String", "BASE_URL", ...)에서
envProperties.getProperty("BASE_URL")에 대한 기본값 처리가 빠져 null이 문자열로 들어갈 수 있으니
buildConfigField 호출부(심볼: buildConfigField, 상수: BASE_URL, 호출:
envProperties.getProperty("BASE_URL"))에 elvis 연산자(?:)로 안전한 기본값을 추가하세요; 예를 들어 개발
환경에서 사용할 로컬 엔드포인트("http://localhost:8080")를 기본값으로 사용하도록 변경하여
API_USERNAME/API_PASSWORD와 동일한 방식으로 처리하세요.
In `@app/src/main/java/woowacourse/shopping/data/remote/api/CouponApi.kt`:
- Line 1: The file contains a leading BOM before the package declaration
'package woowacourse.shopping.data.remote.api'; remove the invisible BOM
character by re-saving CouponApi.kt as UTF-8 without BOM (or remove the BOM byte
manually) so the package line starts immediately with 'package' and re-run
formatting/analysis to confirm no hidden characters remain.
In
`@app/src/main/java/woowacourse/shopping/data/repository/cart/CartRepositoryImpl.kt`:
- Line 27: The getAllCartItems implementation currently requests an unbounded
page size using Int.MAX_VALUE which can overload client/server; modify
CartRepositoryImpl.getAllCartItems to use a safe capped page size (e.g., a
constant like DEFAULT_MAX_PAGE_SIZE or a configurable value) or iterate
paginated calls to cartRemoteDataSource.getCartItems(offset, limit) until all
items are fetched, ensuring you replace the Int.MAX_VALUE call with the chosen
limit and handle repeated requests if full retrieval is required.
- Around line 33-36: The current flow calls
cartRemoteDataSource.addCartItem(...) then re-queries with
getAllCartItems().findByProductId(product.id) which can return the wrong cart
entry under concurrent or pre-existing-product conditions; change the
implementation to obtain the created cart item's identifier directly from the
add-call instead of reverse-lookup: update cartRemoteDataSource.addCartItem to
return the created CartItem or its id (or read the created resource id from the
API response / Location header) and return that id, keeping
_cartEvents.emit(Unit) as-is; if you cannot change the remote signature, make
addCartItem accept/return a unique client-generated token (e.g., requestUuid)
and match on that token to reliably identify the newly-created cart entry.
In
`@app/src/main/java/woowacourse/shopping/data/repository/CouponRepositoryImpl.kt`:
- Around line 11-15: In CouponRepositoryImpl.getAvailableCoupons(), do not
swallow all exceptions; narrow the catch to expected network/IO exceptions or
rethrow coroutine cancellation signals: catch and rethrow CancellationException
(or check isActive) and only handle specific exceptions from
couponRemoteDataSource.getCoupons(), then map with CouponMapper.toDomain and
return emptyList() only for recoverable errors; ensure CancellationException and
other unchecked exceptions propagate instead of being converted to an empty
list.
In `@app/src/main/java/woowacourse/shopping/ui/cart/CartViewModel.kt`:
- Around line 230-239: refreshCartItems does not reconcile selectedItemsState
with the current set of cart IDs, so removed items can remain selected; after
obtaining allResult (via cartRepository.getAllCartItems()) and before or
immediately after updating allCartItemsState/pagedCartItemsState, compute the
current cartId set from allResult.values and update selectedItemsState to the
intersection of its previous value and that cartId set (i.e., remove any
selected ids that no longer exist) so selection stays consistent with
refreshCartItems, pagedCartItemsState, and allCartItemsState.
- Around line 83-88: The code is calling refresh() both directly in action
handlers (e.g., refreshCartItems() inside methods that modify cart items) and
via the cartRepository.cartEvents collector which already triggers refresh();
remove the direct calls from the action handlers and let the cartEvents
subscription (inside viewModelScope.launch that calls refresh()) be the single
source of truth for reloading. Keep the initial refresh() on ViewModel init,
ensure action handlers only perform repository updates (e.g., calls on
cartRepository like add/remove/update) and that those repository methods emit
cartEvents so the existing collector will call refresh(), and remove any
remaining direct calls to refreshCartItems()/refresh() in those handlers.
In `@app/src/main/java/woowacourse/shopping/ui/common/SettingsScreen.kt`:
- Around line 65-72: The onCheckedChange block updates NotificationSettings and
only cancels schedules when unchecked but does nothing when re-enabled; update
the behavior so that when checked == true you schedule or reschedule reminders
(e.g., call PaymentReminderScheduler.schedule(context) and/or
NotificationHelper.schedule(context) after
NotificationSettings.setNotificationEnabled) and when unchecked keep the
existing cancel calls, and move this orchestration out of the Composable into a
ViewModel or a dedicated UseCase so the UI (onCheckedChange) only dispatches the
new enabled state while the ViewModel/UseCase (e.g.,
NotificationSettingsViewModel or NotificationUseCase) performs the scheduling
logic and checks for outstanding unpaid items before scheduling.
In `@app/src/main/java/woowacourse/shopping/ui/MainActivity.kt`:
- Around line 55-64: The loop currently calls
navController.navigate(PaymentRoute(selectedItemIds = pendingSelectedItemIds))
repeatedly which can push duplicate Payment destinations; change the logic so
you check navController.currentBackStackEntry?.destination?.route (and the
navigated flag) first and only invoke navController.navigate once when Payment
is not already present, or use a single-top/navigation option (e.g.,
launchSingleTop/popUpTo behavior) when calling navController.navigate to prevent
duplicate stacking; update the code around the loop variables navigated,
timeoutMillis, currentRoute, PaymentRoute and pendingSelectedItemIds to ensure
navigate is invoked at most once per navigation attempt.
- Around line 68-70: The catch block currently swallows all exceptions (catch
(e: Exception) { delay(200) }) which hides failure causes and prevents proper
handling; update the catch to log the exception (e.g., using Log.e or Timber)
including e.message/stacktrace, and implement a retry decision: inspect the
caught exception (e) and only perform delay(200) and retry for transient errors,
otherwise log and rethrow or abort navigation; ensure this change is applied in
the coroutine/handler around the navigation call in MainActivity.kt so the
exception is visible and non-retryable errors are not silently ignored.
In `@app/src/main/java/woowacourse/shopping/ui/navigation/AppNavHost.kt`:
- Around line 145-146: 현재 nav 이동 시 CartUiState.Success가 아니면 selectedIds가
emptyList로 대체되어 유실될 수 있으니, 결제 네비게이션에서는 uiState를 직접 읽지 말고 선택 아이디의 단일 신뢰 소스(예:
cartViewModel의 별도 프로퍼티/함수)에서 값을 가져오거나 선택 이벤트에서 selectedIds를 캡처해 전달하세요; 예를 들어
cartViewModel에 selectedItemIds: StateFlow/LiveData 또는 getSelectedItemIds():
List<Int> 같은 접근자를 추가하거나, 선택을 처리하는 곳에서 selectedItems를 읽어
PaymentRoute(selectedItemIds = ...)로 넘기도록 변경하여 CartUiState.Success 체크로 인한 빈 리스트
대체를 방지하세요.
In `@app/src/main/java/woowacourse/shopping/ui/payment/PaymentScreen.kt`:
- Around line 72-74: The call to PaymentReminderScheduler.schedule in
PaymentScreen is using a hardcoded test delay (10 * 1000L) which causes
too-early reminders; replace this magic value with a configurable constant or
resource (e.g., REMINDER_DELAY_MS) or read from app settings, and use that
constant when calling PaymentReminderScheduler.schedule(context, reminderDelay,
selectedItemIds); update its declaration (in the same file or a central config)
and adjust any tests to override the config rather than changing the production
call.
In `@app/src/main/java/woowacourse/shopping/ui/payment/PaymentViewModel.kt`:
- Around line 94-99: The empty-selection branch currently emits a message but
continues executing order and navigation; update the selectedIds.isEmpty()
branch in PaymentViewModel to emit PaymentUiEvent.ShowMessage("선택된 상품이 없습니다.")
and then immediately return so cartRepository.order(selectedIds) and subsequent
_uiEvent emissions never run for empty selections; after the change, add a
UI/unit test that verifies when selectedIds is empty the ShowMessage is emitted
once and cartRepository.order and NavigateToProductList are not invoked.
- Around line 44-48: The refresh() and onClickPay() flows lack exception
handling causing the UI to stay in Loading or emit wrong events; wrap repository
calls (cartRepository.getAllCartItems(), couponRepository.getAvailableCoupons(),
orderRepository.order()) in try/catch, on failure restore _uiState to a failure
state (e.g., PaymentUiState.Error with message) or revert to the previous
success state (use previousSelectedCoupon captured before setting Loading) and
ensure onClickPay() does not emit success/navigation events when an exception
occurs; add unit tests for network failure scenarios to assert
PaymentUiState.Error is set and navigation events are not emitted.
In `@app/src/main/java/woowacourse/shopping/ui/util/NotificationSettings.kt`:
- Around line 5-29: NotificationSettings가 UI 레이어에서 SharedPreferences를 직접 다루고
있으므로 데이터 레이어로 이동시키고 책임을 분리하세요: NotificationSettings 객체와 그
메소드(isNotificationEnabled, setNotificationEnabled, markReminderScheduled,
hasScheduledReminder)를 data 계층의 인터페이스(예: NotificationPreferences 또는
NotificationDataSource)로 추출한 뒤 SharedPreferences 접근 구현을 그 구현체에 두고 리포지토리/데이터소스
인터페이스를 ViewModel 등에서 생성자 주입으로 사용하도록 변경하세요; 이렇게 하면 저장 방식(DataStore 등) 변경과 단위 테스트가
용이해집니다.
In `@gradle/libs.versions.toml`:
- Line 23: The libs.versions.toml entry navigation = "2.8.9" may reference a
non-existent artifact version; locate the catalog alias `navigation` in
gradle/libs.versions.toml and check which exact Maven artifact it maps to (e.g.,
androidx.navigation:navigation-runtime-ktx or
androidx.navigation:navigation-compose), then verify that artifact/version
exists on Maven Central; if 2.8.9 is not published for that artifact, change the
`navigation` value to a published version (or correct the artifact mapping) so
the dependency coordinates resolve during build.
---
Nitpick comments:
In `@app/src/main/java/woowacourse/shopping/constant/ShoppingColor.kt`:
- Around line 9-14: The six hardcoded ARGB color constants in ShoppingColor.kt
(PAYMENT_BUTTON_COLOR, PAYMENT_DIVIDER_COLOR, PAYMENT_CARD_BORDER_COLOR,
PAYMENT_SUMMARY_BACKGROUND_COLOR, PAYMENT_DESCRIPTION_GRAY, PAYMENT_TITLE_GRAY)
should be moved out of raw ints and integrated into the app theme: create named
color tokens in the central theme (light/dark variants) and expose them via
Material3 ColorScheme or your Theme object, then replace usages of these
constants to read from the ColorScheme (or Theme.colors) so dark mode and
centralized overrides work; update any callers that reference these constants to
use the themed color tokens instead.
In `@app/src/main/java/woowacourse/shopping/data/remote/dto/CouponResponseDto.kt`:
- Line 10: The DTO field discountType is currently a raw String which risks
invalid values slipping past compile-time; update the design by either 1) making
CouponResponseDto use a dedicated enum (e.g., DiscountType) to enforce
type-safety at the DTO boundary, or 2) if the API can return arbitrary strings,
keep String but add strict validation and clear error handling in CouponMapper
(the mapping function that converts CouponResponseDto -> domain coupon) to
translate/normalize strings into the domain DiscountType enum and surface
mapping errors early; locate CouponResponseDto and the CouponMapper mapping
logic and implement one of these approaches so invalid types are detected and
handled at the mapper/domain boundary rather than silently propagated.
In `@app/src/main/java/woowacourse/shopping/domain/exception/ProductException.kt`:
- Around line 20-22: EmptyBody is declared as a plain object while other sealed
ProductException subtypes (ServerError, NetworkError, NotFound) are data
classes; change EmptyBody to a data object (i.e., make it a "data object
EmptyBody : ProductException(...)") so it benefits from generated
toString/equals/hashCode and maintains a consistent sealed-hierarchy pattern
with the other types.
In
`@app/src/main/java/woowacourse/shopping/domain/model/coupon/CouponCalculator.kt`:
- Around line 120-123: The percent-discount calculation currently truncates
fractions using (subtotal * rate).toInt(), losing cents; change this to use
rounding by replacing that expression with (subtotal * rate).roundToInt() (and
add the kotlin.math.roundToInt import), keep appliedDiscount =
discount.coerceAtMost(subtotal) and the returned CouponApplyResult as-is; if
truncation was intentional instead, add a clear inline comment above the
rate/discount calculation in CouponCalculator.kt explaining that .toInt()
(flooring) is deliberate.
In `@app/src/main/java/woowacourse/shopping/domain/model/coupon/CouponTypes.kt`:
- Around line 14-20: The fromCode function in CouponTypes.kt currently matches
codes case-sensitively and will treat " fixed5000 " or "fixed5000" as UNKNOWN;
normalize the input first by applying trim() and uppercase (e.g., val normalized
= code.trim().uppercase()) and then use normalized in the when expression to map
to FIXED5000, BOGO, FREESHIPPING, MIRACLESALE, or UNKNOWN; update any tests or
callers if they relied on exact-case behavior.
In `@app/src/main/java/woowacourse/shopping/ui/common/SettingsScreen.kt`:
- Around line 36-38: 현재 SettingsScreen의 notificationEnabled 변수가 remember {
mutableStateOf(NotificationSettings.isNotificationEnabled(context)) }로 초기화되어 초기
Composition 시 값만 읽히므로 외부에서 설정이 변경되어도 화면이 갱신되지 않습니다;
NotificationSettings.isNotificationEnabled(context) 호출을 remember 안에서 제거하고,
NotificationSettings에 StateFlow<Boolean> 또는 LiveData를 추가(예:
NotificationSettings.notificationEnabledFlow)하여 SettingsScreen에서 해당
Flow/LiveData를 수집해 UI 상태로 변환(collectAsState 또는 observeAsState 등)하도록 변경하거나,
NotificationSettings에 콜백/리스너를 제공해 변경시 mutableStateOf를 업데이트하도록 구현해 변경을 관찰 가능하게
만드세요.
In `@app/src/main/java/woowacourse/shopping/ui/payment/PaymentUiState.kt`:
- Line 15: PaymentUiState currently hardcodes shippingFee = 3000 in the data
class; remove the business-logic default and instead accept shippingFee as a
provided value (no hardcoded default) so the UI state is populated by the
domain/service layer that calculates fees, or if it truly is a constant move the
value to a dedicated constant (e.g., ShoppingConstants.SHIPPING_FEE) and
reference that constant from PaymentUiState; update any creators/factories that
instantiate PaymentUiState (tests, viewmodels) to pass the correct shippingFee
from the order calculation or constant.
- Around line 9-17: The Success data class currently supplies default zeros for
subtotal, couponDiscount, and totalPrice which can mask missing calculations;
remove those default values (make subtotal, couponDiscount, totalPrice
non-optional constructor parameters) or provide a single factory method (e.g., a
Success.Companion.createFrom(cartItems, availableCoupons, selectedCoupon,
shippingFee)) that computes and returns a fully populated Success instance so
callers cannot accidentally construct an incomplete UI state; update call sites
that instantiate Success to use the computed values or the factory.
In
`@app/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailViewModel.kt`:
- Around line 94-107: The addToCart() function currently uses a hardcoded
delay(1000) after emitting _uiEvent ShowSnackbar which can cause unintended
cancellation or poor UX; extract the timeout into a named constant (e.g.,
AUTO_NAV_DELAY_MS) and replace delay(1000) with that constant, and instead of
auto-navigating unconditionally from viewModelScope, change the flow to either
(A) expose a snackbar event with an action button that triggers
_uiEvent.NavigateToCart when the user taps it, or (B) perform the delayed
navigation using viewModelScope.launch { withContext(NonCancellable) {
delay(AUTO_NAV_DELAY_MS); _uiEvent.emit(UiEvent.NavigateToCart) } } if you
intend navigation to proceed even if the caller cancels; update references to
_uiState, cartRepository, _uiEvent, and addToCart() accordingly so the intent is
clear.
In `@app/src/main/java/woowacourse/shopping/ui/productList/ProductListUiState.kt`:
- Line 7: Change the singleton declaration back to a data object to keep
consistency and retain generated equals/hashCode/toString: replace the current
"object Loading : ProductListUiState" with "data object Loading :
ProductListUiState" in the ProductListUiState declaration (refer to the
ProductListUiState.Loading symbol) so it uses Kotlin 1.9's data object behavior.
In
`@app/src/main/java/woowacourse/shopping/ui/productList/ProductListViewModel.kt`:
- Around line 64-70: The addProduct method calls refreshCart() directly and
cartEvents also triggers refreshCart(), causing duplicate fetches; remove the
direct refreshCart() call inside addProduct (keep emitting
_uiEvent.ShowSnackbar) and rely on the existing cartEvents flow to refresh the
cart, or alternatively implement a debounce/guard if you prefer event-driven +
immediate refresh; update the addProduct function (referencing addProduct,
refreshCart, and cartEvents) to eliminate the redundant refresh invocation.
In `@app/src/main/java/woowacourse/shopping/ui/util/NotificationSettings.kt`:
- Around line 10-28: Refactor repeated context.getSharedPreferences calls in
NotificationSettings.kt by caching a SharedPreferences instance and use it
across functions: create a private lazy or initialized property (e.g., private
lateinit or private val sharedPrefs) that is set once (via an init method that
accepts Context or by providing Context to an initialize(context) function) and
then replace calls inside isNotificationEnabled, setNotificationEnabled,
markReminderScheduled, and hasScheduledReminder to use that cached sharedPrefs;
alternatively consider a Context extension that returns the prefs to keep call
sites concise—ensure initialization happens before any getter/setter is used to
avoid NPEs.
In `@app/src/main/java/woowacourse/shopping/ui/util/PaymentReminderReceiver.kt`:
- Around line 9-20: onReceive in PaymentReminderReceiver runs on the main thread
with a short timeout, so wrap the notification work in a background task and add
robust exception handling: call goAsync() inside
PaymentReminderReceiver.onReceive to obtain a PendingResult, then offload
NotificationHelper.createNotificationChannel and
NotificationHelper.showPaymentReminder to a background thread/coroutine (or
WorkManager) and call finish() on the PendingResult when done; surround the
background work with try/catch to handle and log exceptions (e.g., permission
issues, resource errors) and ensure NotificationSettings.isNotificationEnabled
is checked before dispatching work; also validate the intent extra
(PaymentReminderScheduler.EXTRA_SELECTED_ITEM_IDS) and handle null/empty safely.
In `@app/src/main/java/woowacourse/shopping/ui/util/PaymentReminderScheduler.kt`:
- Around line 41-46: The current use of AlarmManager.setAndAllowWhileIdle in
PaymentReminderScheduler (the alarmManager.setAndAllowWhileIdle call) is inexact
and may be deferred by Doze; if you need guaranteed exact firing change the call
to AlarmManager.setExactAndAllowWhileIdle and add handling to request/declare
the SCHEDULE_EXACT_ALARM permission (and fall back to setAndAllowWhileIdle when
the permission isn't granted). Update the scheduler logic around the existing
alarmManager.cancel(...) / set... sequence to perform the permission check,
request or gracefully degrade to the inexact setter, and keep the same
PendingIntent usage (PaymentReminderScheduler / the pendingIntent creation) so
behavior remains consistent.
In
`@app/src/test/kotlin/woowacourse/shopping/domain/model/coupon/CouponCalculatorTest.kt`:
- Around line 161-193: Add boundary/time-range tests for CouponCalculator.apply:
currently MIRACLESALE_오전_적용 and PERCENT_시간제한_없으면_항상_적용 use coupons without
availableStartTime/availableEndTime so they only cover "no time limit". Create
additional unit tests that construct Coupon instances with availableStartTime
and availableEndTime set and call CouponCalculator.apply(cart, coupon, now=...)
to assert applied/discount for (1) times inside the range, (2) times just
outside the range (boundary minutes), (3) an overnight range that crosses
midnight (e.g., availableStartTime=22:00, availableEndTime=06:00) with now at
23:00 and 05:00, and (4) an expired coupon (expireAt before now) to assert not
applied; reference the Coupon class fields
availableStartTime/availableEndTime/expireAt and the CouponCalculator.apply(...)
and existing test names to add these cases.
🪄 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: 332d886b-29c4-4090-8bb3-f01945534762
📒 Files selected for processing (64)
.env.gitignoreREADME.mdapp/build.gradle.ktsapp/src/debug/AndroidManifest.xmlapp/src/debug/java/woowacourse/shopping/mock/MockServerInitializer.ktapp/src/debug/java/woowacourse/shopping/mock/ShoppingMockServer.ktapp/src/main/AndroidManifest.xmlapp/src/main/java/woowacourse/shopping/constant/Format.ktapp/src/main/java/woowacourse/shopping/constant/ShoppingColor.ktapp/src/main/java/woowacourse/shopping/data/datasource/cart/CartRemoteDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/datasource/coupon/CouponRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/datasource/coupon/CouponRemoteDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/datasource/product/ProductRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/datasource/product/ProductRemoteDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/mapper/CartItemMapper.ktapp/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.ktapp/src/main/java/woowacourse/shopping/data/remote/RetrofitProvider.ktapp/src/main/java/woowacourse/shopping/data/remote/api/CartApi.ktapp/src/main/java/woowacourse/shopping/data/remote/api/CouponApi.ktapp/src/main/java/woowacourse/shopping/data/remote/dto/CartItemCountResponseDto.ktapp/src/main/java/woowacourse/shopping/data/remote/dto/CouponResponseDto.ktapp/src/main/java/woowacourse/shopping/data/repository/CouponRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/cart/CartRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/product/RemoteProductRepository.ktapp/src/main/java/woowacourse/shopping/di/AppContainer.ktapp/src/main/java/woowacourse/shopping/domain/exception/ProductException.ktapp/src/main/java/woowacourse/shopping/domain/model/cart/CartItems.ktapp/src/main/java/woowacourse/shopping/domain/model/coupon/Coupon.ktapp/src/main/java/woowacourse/shopping/domain/model/coupon/CouponCalculator.ktapp/src/main/java/woowacourse/shopping/domain/model/coupon/CouponType.ktapp/src/main/java/woowacourse/shopping/domain/model/coupon/CouponTypes.ktapp/src/main/java/woowacourse/shopping/domain/model/product/Products.ktapp/src/main/java/woowacourse/shopping/domain/repository/CartRepository.ktapp/src/main/java/woowacourse/shopping/domain/repository/CouponRepository.ktapp/src/main/java/woowacourse/shopping/domain/repository/ProductRepository.ktapp/src/main/java/woowacourse/shopping/ui/MainActivity.ktapp/src/main/java/woowacourse/shopping/ui/UiEvent.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartActivity.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartUiState.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartViewModel.ktapp/src/main/java/woowacourse/shopping/ui/common/SettingsScreen.ktapp/src/main/java/woowacourse/shopping/ui/navigation/AppNavHost.ktapp/src/main/java/woowacourse/shopping/ui/navigation/Routes.ktapp/src/main/java/woowacourse/shopping/ui/payment/PaymentScreen.ktapp/src/main/java/woowacourse/shopping/ui/payment/PaymentUiState.ktapp/src/main/java/woowacourse/shopping/ui/payment/PaymentViewModel.ktapp/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailActivity.ktapp/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailUiState.ktapp/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailViewModel.ktapp/src/main/java/woowacourse/shopping/ui/productList/ProductListActivity.ktapp/src/main/java/woowacourse/shopping/ui/productList/ProductListScreen.ktapp/src/main/java/woowacourse/shopping/ui/productList/ProductListUiState.ktapp/src/main/java/woowacourse/shopping/ui/productList/ProductListViewModel.ktapp/src/main/java/woowacourse/shopping/ui/util/NotificationHelper.ktapp/src/main/java/woowacourse/shopping/ui/util/NotificationSettings.ktapp/src/main/java/woowacourse/shopping/ui/util/PaymentReminderReceiver.ktapp/src/main/java/woowacourse/shopping/ui/util/PaymentReminderScheduler.ktapp/src/main/res/drawable/ic_alarm.xmlapp/src/main/res/drawable/ic_settings.xmlapp/src/main/res/xml/network_security_config.xmlapp/src/test/kotlin/woowacourse/shopping/domain/model/coupon/CouponCalculatorTest.ktbuild.gradle.ktsgradle/libs.versions.toml
💤 Files with no reviewable changes (8)
- app/src/main/java/woowacourse/shopping/ui/cart/CartActivity.kt
- .env
- app/src/main/java/woowacourse/shopping/ui/productList/ProductListActivity.kt
- app/src/main/res/xml/network_security_config.xml
- app/src/debug/java/woowacourse/shopping/mock/MockServerInitializer.kt
- app/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailActivity.kt
- app/src/debug/java/woowacourse/shopping/mock/ShoppingMockServer.kt
- app/src/debug/AndroidManifest.xml
| .DS_Store | ||
|
|
||
| .env | ||
| app/src/main/res/xml/network_security_config.xml |
There was a problem hiding this comment.
network_security_config.xml을 .gitignore에 넣으면 빌드 재현성이 깨질 수 있습니다.
Line 42 항목 때문에 팀원/CI 환경에서 보안 설정 리소스가 누락될 위험이 큽니다. 해당 파일은 버전 관리에 포함하고, 민감값이 있다면 별도 주입 방식으로 분리하는 편이 안전합니다.
🤖 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 @.gitignore at line 42, 현재 .gitignore에 추가된
app/src/main/res/xml/network_security_config.xml 항목 때문에 빌드/CI 환경에서 네트워크 보안 리소스가
누락될 위험이 있으니 해당 파일을 .gitignore에서 제거하고 저장소에 포함시켜 버전관리를 복원하세요; 만약
network_security_config.xml 내부에 민감한 값(인증서, 도메인 키 등)이 있다면 파일 자체를 커밋하지 말고, 대신
network_security_config.xml에서 민감값을 분리하여 환경별 주입(빌드 변수, CI 시크릿, 또는 리소스 템플릿 + 변환
스텝)으로 대체하도록 리팩토링하세요.
Comment on lines
36
to
40
| buildConfigField( | ||
| "String", | ||
| "BASE_URL", | ||
| "\"${envProperties.getProperty("BASE_URL")}\"" | ||
| "\"${envProperties.getProperty("BASE_URL")}\"", | ||
| ) |
There was a problem hiding this comment.
BASE_URL에 대한 기본값(fallback) 처리가 누락되었습니다.
Line 39에서 .env 파일에 BASE_URL이 정의되지 않은 경우 null이 문자열로 들어가게 됩니다. 45번, 51번 라인의 API_USERNAME, API_PASSWORD와 달리 elvis 연산자(?:)를 사용한 기본값 처리가 없습니다.
이 문제를 해결하려면 어떤 기본값을 제공하는 것이 적절할까요? 개발 환경에서 사용할 수 있는 로컬 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/build.gradle.kts` around lines 36 - 40, buildConfigField("String",
"BASE_URL", ...)에서 envProperties.getProperty("BASE_URL")에 대한 기본값 처리가 빠져 null이
문자열로 들어갈 수 있으니 buildConfigField 호출부(심볼: buildConfigField, 상수: BASE_URL, 호출:
envProperties.getProperty("BASE_URL"))에 elvis 연산자(?:)로 안전한 기본값을 추가하세요; 예를 들어 개발
환경에서 사용할 로컬 엔드포인트("http://localhost:8080")를 기본값으로 사용하도록 변경하여
API_USERNAME/API_PASSWORD와 동일한 방식으로 처리하세요.
| @@ -0,0 +1,8 @@ | |||
| package woowacourse.shopping.data.remote.api | |||
There was a problem hiding this comment.
파일 시작 BOM 문자를 제거해 주세요.
Line 1의 package 앞 BOM 문자는 일부 툴(포맷터/정적분석)에서 불필요한 노이즈나 실패 원인이 됩니다. UTF-8(BOM 없음)으로 저장하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/data/remote/api/CouponApi.kt` at line
1, The file contains a leading BOM before the package declaration 'package
woowacourse.shopping.data.remote.api'; remove the invisible BOM character by
re-saving CouponApi.kt as UTF-8 without BOM (or remove the BOM byte manually) so
the package line starts immediately with 'package' and re-run
formatting/analysis to confirm no hidden characters remain.
| override suspend fun getCartItemsCount(): Int = cartRemoteDataSource.getCartItemsCount() | ||
|
|
||
| override suspend fun getAllCartItems(): PagedCartItems = cartRemoteDataSource.getCartItems(0, Int.MAX_VALUE).toDomain() | ||
| override suspend fun getAllCartItems(): CartItems = cartRemoteDataSource.getCartItems(0, Int.MAX_VALUE).toCartItems() |
There was a problem hiding this comment.
Int.MAX_VALUE 페이지 크기 요청은 장애 유발 포인트가 됩니다.
Line 27의 사실상 무제한 조회는 서버/클라이언트 모두에 부담이 큽니다. 상한을 두고 필요한 범위만 읽는 방식으로 제한하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/woowacourse/shopping/data/repository/cart/CartRepositoryImpl.kt`
at line 27, The getAllCartItems implementation currently requests an unbounded
page size using Int.MAX_VALUE which can overload client/server; modify
CartRepositoryImpl.getAllCartItems to use a safe capped page size (e.g., a
constant like DEFAULT_MAX_PAGE_SIZE or a configurable value) or iterate
paginated calls to cartRemoteDataSource.getCartItems(offset, limit) until all
items are fetched, ensuring you replace the Int.MAX_VALUE call with the chosen
limit and handle repeated requests if full retrieval is required.
Comment on lines
33
to
+36
| cartRemoteDataSource.addCartItem(product.id, quantity) | ||
| _cartEvents.emit(Unit) | ||
| return getAllCartItems().findByProductId(product.id)?.id | ||
| ?: throw IllegalStateException("장바구니에 상품이 추가되지 않았습니다.") |
There was a problem hiding this comment.
추가 직후 cartId를 역조회하는 방식은 식별 정확도가 떨어질 수 있습니다.
Line 35에서 product.id로 전체 장바구니를 다시 조회해 id를 찾으면, 동일 상품 기존 항목/동시 변경 상황에서 의도한 cartId가 아닐 수 있습니다. 추가 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/repository/cart/CartRepositoryImpl.kt`
around lines 33 - 36, The current flow calls
cartRemoteDataSource.addCartItem(...) then re-queries with
getAllCartItems().findByProductId(product.id) which can return the wrong cart
entry under concurrent or pre-existing-product conditions; change the
implementation to obtain the created cart item's identifier directly from the
add-call instead of reverse-lookup: update cartRemoteDataSource.addCartItem to
return the created CartItem or its id (or read the created resource id from the
API response / Location header) and return that id, keeping
_cartEvents.emit(Unit) as-is; if you cannot change the remote signature, make
addCartItem accept/return a unique client-generated token (e.g., requestUuid)
and match on that token to reliably identify the newly-created cart entry.
Comment on lines
+72
to
+74
| //PaymentReminderScheduler.schedule(context, 5 * 60 * 1000L, selectedItemIds) | ||
| PaymentReminderScheduler.schedule(context, 10 * 1000L, selectedItemIds) | ||
| } |
There was a problem hiding this comment.
리마인더 지연 시간이 테스트 값(10초)으로 고정되어 있습니다.
Line 73은 사용자에게 너무 이른 알림을 유발할 가능성이 큽니다. 실제 요구사항 기준 지연값으로 분리(예: 상수/설정화)했는지 다시 확인해보세요.
🤖 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/payment/PaymentScreen.kt` around
lines 72 - 74, The call to PaymentReminderScheduler.schedule in PaymentScreen is
using a hardcoded test delay (10 * 1000L) which causes too-early reminders;
replace this magic value with a configurable constant or resource (e.g.,
REMINDER_DELAY_MS) or read from app settings, and use that constant when calling
PaymentReminderScheduler.schedule(context, reminderDelay, selectedItemIds);
update its declaration (in the same file or a central config) and adjust any
tests to override the config rather than changing the production call.
Member
There was a problem hiding this comment.
테스트용으로 넣은 값이 프로덕션 환경까지 넘어가는 경우 앱은 치명적일 수 있습니다. 값을 상수화하는 것도 좋아보이네요. 👍
Comment on lines
+44
to
+48
| viewModelScope.launch { | ||
| val previousSelectedCoupon = (_uiState.value as? PaymentUiState.Success)?.selectedCoupon | ||
| _uiState.value = PaymentUiState.Loading | ||
| val allCart = cartRepository.getAllCartItems() | ||
|
|
There was a problem hiding this comment.
저장소 호출 실패 시 상태 복구 경로가 필요합니다.
getAllCartItems(), getAvailableCoupons(), order() 실패 시 예외 처리 경로가 없어 결제 화면이 로딩 상태에 머물거나 이벤트가 누락될 수 있습니다.
여기서는 “실패를 UI 상태/이벤트로 변환”하는 최소 경로를 먼저 잡아보는 게 좋겠습니다.
-
refresh()에서 실패 시PaymentUiState.Error또는 메시지 이벤트로 전환
-
onClickPay()에서 실패 시 성공 이벤트/네비게이션 방지
-
- 네트워크 실패 케이스 단위 테스트로 회귀 방지
Also applies to: 58-59, 97-99
🤖 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/payment/PaymentViewModel.kt` around
lines 44 - 48, The refresh() and onClickPay() flows lack exception handling
causing the UI to stay in Loading or emit wrong events; wrap repository calls
(cartRepository.getAllCartItems(), couponRepository.getAvailableCoupons(),
orderRepository.order()) in try/catch, on failure restore _uiState to a failure
state (e.g., PaymentUiState.Error with message) or revert to the previous
success state (use previousSelectedCoupon captured before setting Loading) and
ensure onClickPay() does not emit success/navigation events when an exception
occurs; add unit tests for network failure scenarios to assert
PaymentUiState.Error is set and navigation events are not emitted.
Comment on lines
+94
to
+99
| if (selectedIds.isEmpty()) { | ||
| _uiEvent.emit(PaymentUiEvent.ShowMessage("선택된 상품이 없습니다.")) | ||
| } | ||
| cartRepository.order(selectedIds) | ||
| _uiEvent.emit(PaymentUiEvent.ShowMessage("주문이 완료되었습니다")) | ||
| _uiEvent.emit(PaymentUiEvent.NavigateToProductList) |
There was a problem hiding this comment.
선택 상품이 없을 때 결제 플로우를 즉시 중단해 주세요.
Line 95에서 안내 메시지를 보낸 뒤에도 Line 97~99가 계속 실행됩니다. 이 경우 빈 주문 요청/완료 메시지/화면 이동까지 이어질 수 있어요.
이 지점에서 “메시지 전송 후 return”이 맞는지 먼저 확인해보면 어떨까요?
-
selectedIds.isEmpty()분기에서 함수 종료
-
- 종료 후에는 주문 API/성공 이벤트가 절대 실행되지 않는지 점검
-
- 빈 선택 케이스 UI 테스트 1개 추가
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/ui/payment/PaymentViewModel.kt` around
lines 94 - 99, The empty-selection branch currently emits a message but
continues executing order and navigation; update the selectedIds.isEmpty()
branch in PaymentViewModel to emit PaymentUiEvent.ShowMessage("선택된 상품이 없습니다.")
and then immediately return so cartRepository.order(selectedIds) and subsequent
_uiEvent emissions never run for empty selections; after the change, add a
UI/unit test that verifies when selectedIds is empty the ShowMessage is emitted
once and cartRepository.order and NavigateToProductList are not invoked.
Comment on lines
+5
to
+29
| object NotificationSettings { | ||
| private const val PREFS_NAME = "settings" | ||
| private const val KEY_NOTIFICATION = "notification" | ||
| private const val KEY_HAS_SCHEDULED_REMINDER = "has_scheduled_reminder" | ||
|
|
||
| fun isNotificationEnabled(context: Context): Boolean { | ||
| val sp = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
| return sp.getBoolean(KEY_NOTIFICATION, true) | ||
| } | ||
|
|
||
| fun setNotificationEnabled(context: Context, enabled: Boolean) { | ||
| val sp = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
| sp.edit().putBoolean(KEY_NOTIFICATION, enabled).apply() | ||
| } | ||
|
|
||
| fun markReminderScheduled(context: Context, scheduled: Boolean) { | ||
| val sp = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
| sp.edit().putBoolean(KEY_HAS_SCHEDULED_REMINDER, scheduled).apply() | ||
| } | ||
|
|
||
| fun hasScheduledReminder(context: Context): Boolean { | ||
| val sp = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
| return sp.getBoolean(KEY_HAS_SCHEDULED_REMINDER, false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
알림 설정의 레이어 위치를 재검토해보세요.
PR 설명에서 요청하신 "알림 설정의 상태 관리 위치와 책임 분리"와 직접적으로 관련된 부분입니다.
현재 NotificationSettings는 ui.util 패키지에 위치하며 SharedPreferences를 직접 다룹니다. 다음 질문들을 고민해보세요:
-
레이어 분리: UI 레이어에서 데이터 저장소(SharedPreferences)를 직접 접근하는 것이 Clean Architecture 관점에서 적절한가요?
- 데이터 레이어(
data.datasource또는data.repository)로 이동하면 어떤 이점이 있을까요? - 나중에 저장 방식을 변경(예: DataStore)해야 한다면 영향 범위는 어떻게 될까요?
- 데이터 레이어(
-
책임 분리: 이 객체는 순수하게 "설정 값 읽기/쓰기"만 담당하나요, 아니면 비즈니스 로직도 포함되나요?
markReminderScheduled처럼 알림 스케줄링과 관련된 상태를 관리하는 것은 도메인 로직에 가깝지 않나요?
-
테스트 용이성: 현재 구조에서 이 클래스를 사용하는 ViewModel이나 다른 컴포넌트를 테스트할 때 어려움은 없을까요?
Repository 패턴이나 DataSource 인터페이스 적용을 고려해보시고, 각 레이어의 책임과 의존성 방향을 다시 한 번 점검해보세요.
🤖 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/util/NotificationSettings.kt`
around lines 5 - 29, NotificationSettings가 UI 레이어에서 SharedPreferences를 직접 다루고
있으므로 데이터 레이어로 이동시키고 책임을 분리하세요: NotificationSettings 객체와 그
메소드(isNotificationEnabled, setNotificationEnabled, markReminderScheduled,
hasScheduledReminder)를 data 계층의 인터페이스(예: NotificationPreferences 또는
NotificationDataSource)로 추출한 뒤 SharedPreferences 접근 구현을 그 구현체에 두고 리포지토리/데이터소스
인터페이스를 ViewModel 등에서 생성자 주입으로 사용하도록 변경하세요; 이렇게 하면 저장 방식(DataStore 등) 변경과 단위 테스트가
용이해집니다.
| room = "2.8.3" | ||
| kotlinx-serialization = "1.11.0" | ||
| okhttp = "4.12.0" | ||
| navigation = "2.8.9" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check Maven Central for navigation-compose version and GitHub advisories
# Check Maven Central for available versions
echo "=== Checking Maven Central for androidx.navigation:navigation-compose ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:androidx.navigation+AND+a:navigation-compose&rows=20&wt=json" | jq -r '.response.docs[] | .v' | sort -V | tail -10
# Check for security advisories
echo -e "\n=== Checking for security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: MAVEN, package: "androidx.navigation:navigation-compose") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: woowacourse/android-shopping-order
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
VERSION="2.8.9"
echo "=== Checking Maven Central for AndroidX Navigation artifact versions ==="
for ARTIFACT in navigation navigation-compose; do
echo -e "\n-- ${ARTIFACT} (androidx.navigation:${ARTIFACT}) --"
curl -s "https://search.maven.org/solrsearch/select?q=g:androidx.navigation+AND+a:${ARTIFACT}&rows=20&wt=json" \
| jq -r '.response.docs[].v' | sort -V | tail -20
echo "Check contains ${VERSION} for ${ARTIFACT}:"
curl -s "https://search.maven.org/solrsearch/select?q=g:androidx.navigation+AND+a:${ARTIFACT}+AND+v:${VERSION}&rows=1&wt=json" \
| jq -r '.response.numFound'
done
echo -e "\n=== Checking OSV (GitHub GraphQL) for security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: MAVEN, package: "androidx.navigation:navigation-compose") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}'Repository: woowacourse/android-shopping-order
Length of output: 467
navigation = "2.8.9" 버전 존재 여부 재확인 필요
Maven Central 기준 androidx.navigation:navigation / androidx.navigation:navigation-compose에서 버전 2.8.9는 조회 0건으로 확인됩니다. 해당 좌표로 실제 의존성이 걸려 있다면 빌드/의존성 해소 실패 가능성이 큽니다.
또한 OSV/GitHub advisory 조회에서는 androidx.navigation:navigation-compose 취약점 노드는 없음이었습니다.
gradle/libs.versions.toml에서 navigation 버전이 매핑되는 정확한 아티팩트(예: runtime-ktx 등)를 먼저 확인한 뒤, 그 좌표로 2.8.9의 존재 여부를 다시 확인하거나 존재하는 버전으로 수정하세요.
🤖 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 `@gradle/libs.versions.toml` at line 23, The libs.versions.toml entry
navigation = "2.8.9" may reference a non-existent artifact version; locate the
catalog alias `navigation` in gradle/libs.versions.toml and check which exact
Maven artifact it maps to (e.g., androidx.navigation:navigation-runtime-ktx or
androidx.navigation:navigation-compose), then verify that artifact/version
exists on Maven Central; if 2.8.9 is not published for that artifact, change the
`navigation` value to a published version (or correct the artifact mapping) so
the dependency coordinates resolve during build.
Author
|
안녕하세요 베르! 3,4단계 잘 부탁드립니다 :)
|
SeongHoonC
requested changes
May 25, 2026
Member
SeongHoonC
left a comment
SeongHoonC
left a comment
There was a problem hiding this comment.
안녕하세요 모스!
미션 전반적으로 잘 구현해주셨습니다. 👍
본문 내용에 남겨주신 부분을 고려해 리뷰 남겼습니다.
변경사항이 꽤 많아 이번 단계에서 중요한 내용을 위주로 피드백 남겼습니다.
MVVM 패턴에서 View, ViewModel 의 관심사를 고려했을 때 이벤트를 어떻게 설계하는게 좋을지 고민해보시면 좋을 것 같아요.
반영 후 재요청해주세요. 화이팅입니다 :)
Comment on lines
+72
to
+74
| //PaymentReminderScheduler.schedule(context, 5 * 60 * 1000L, selectedItemIds) | ||
| PaymentReminderScheduler.schedule(context, 10 * 1000L, selectedItemIds) | ||
| } |
Member
There was a problem hiding this comment.
테스트용으로 넣은 값이 프로덕션 환경까지 넘어가는 경우 앱은 치명적일 수 있습니다. 값을 상수화하는 것도 좋아보이네요. 👍
| val message: String, | ||
| ) : PaymentUiEvent | ||
|
|
||
| object NavigateToProductList : PaymentUiEvent |
Member
There was a problem hiding this comment.
UiEvent 의 이름에 대해서 어떻게 생각하시나요?
- View 의 동작을 결정할 때
- 상품 목록으로 이동한다.
- 이벤트만 전달할 때
-
주문에 성공했다.
-
Toast 가 아니라 다이얼로그를 띄운다면 ViewModel 이 수정되어야할까요?
-
주문 성공 시 상세 화면으로 이동하는 것으로 변경되면 ViewModel 이 수정되어야할까요?
ViewModel 에서 동작을 명시한다는 것은 ViewModel 이 View 를 알고있다고 볼 수 있습니다. 그럼 View 를 import 하지 않더라도 영향을 받게됩니다. 알지 않더라도 화면을 변경해야하는거죠. 네비게이션은 누구의 관심사인가요?
Member
There was a problem hiding this comment.
AppNavHost 가 Navigation 뿐만 아니라 event collect, route 에서 값 읽기 뷰모델 생성 등 너무 많은 일을 하고있어요. 화면이 많아지면 많아질수록 이 파일을 변경할 이유가 많아집니다. 네비게이션 책임만 남기고 화면의 책임을 분리해주세요.
Comment on lines
+62
to
+65
| Scaffold( | ||
| snackbarHost = { SnackbarHost(hostState = snackbarHostState) }, | ||
| modifier = Modifier.fillMaxSize(), | ||
| ) { innerPadding -> |
Member
There was a problem hiding this comment.
snackbar 를 띄우기 위한 Scaffold, snackbarHostState 가 반복되고 있어요. 이 Scaffold 는 꼭 화면별로 따로 존재해야할까요? 더 상위에 위치시키고 여기서 showSnackbar 함수만 호출해도 되지않을까요?
Comment on lines
+195
to
+200
| navController.navigate(ProductListRoute) { | ||
| popUpTo<PaymentRoute> { | ||
| inclusive = true | ||
| } | ||
| launchSingleTop = true | ||
| } |
Member
There was a problem hiding this comment.
주문 완료 후 장바구니가 Back Stack에 남습니다. 결제 완료하고 뒤로가기 해보면 아실 수 있을거에요! 지금 남는 이유에 대해 학습하고 코멘트 남긴 후 수정해주세요 😄
| private val _uiState = MutableStateFlow<PaymentUiState>(PaymentUiState.Loading) | ||
| val uiState: StateFlow<PaymentUiState> = _uiState.asStateFlow() | ||
|
|
||
| private val _uiEvent = MutableSharedFlow<PaymentUiEvent>() |
Member
There was a problem hiding this comment.
이벤트 수집이 Lifecycle aware 하게 변경되면 이벤트가 collector 가 없는 백그라운드에서 이벤트가 소실될 수 있을 것 같네요. 즉, 다시 돌아왔을 때 이벤트가 발생하지 않는거죠. 소실되지 않도록 하려면 어떻게 해야할까요?
| @@ -0,0 +1,22 @@ | |||
| package woowacourse.shopping.domain.model.coupon | |||
|
|
|||
| object CouponTypes { | |||
Member
There was a problem hiding this comment.
enum 이나 sealed interface 를 활용해보는건 어떤가요?
Comment on lines
+3
to
+13
| sealed interface UiEvent { | ||
| data class ShowSnackbar( | ||
| val message: String, | ||
| ) : UiEvent | ||
|
|
||
| object NavigateToProductList : UiEvent | ||
|
|
||
| object NavigateToCart : UiEvent | ||
|
|
||
| object NavigateToPayment : UiEvent | ||
| } |
Member
There was a problem hiding this comment.
공통으로 사용할 UiEvent 를 만들고 사용하고 있어요.
특정 화면으로 네비게이션하는 이벤트를 모든 Screen, ViewModel 에서 알고 있어야할지 고민해보면 좋을 것 같아요.
productDetailViewModel.uiEvent.collect { event ->
when (event) {
is UiEvent.ShowSnackbar -> snackbarHostState.showSnackbar(event.message)
UiEvent.NavigateToCart -> navController.navigate(CartRoute)
UiEvent.NavigateToProductList -> Unit
UiEvent.NavigateToPayment -> Unit
}
}| private const val CHANNEL_ID = "payment_reminder_channel" | ||
| private const val CHANNEL_NAME = "결제 알림" | ||
| private const val NOTIF_ID = 2000 | ||
| const val EXTRA_SELECTED_ITEM_IDS = "extra_selected_item_ids" |
Member
There was a problem hiding this comment.
extra_selected_item_ids 키가 여러 곳에 중복되고 있어요. 지금은 문자열이 똑같아서 문제가 발생하지 않지만 PaymentReminderContract 같은 공통된 상수로 빼서 관리할 필요가 있어보이네요
Member
There was a problem hiding this comment.
NotificationHelper가 알림 UI를 조립하는 역할뿐 아니라 MainActivity, action, flags, navigation extra까지 알고 있어요.
알림 클릭 시 어느 화면으로 이동할지에 대한 것도 있어 알림 표시 책임과 화면 이동 책임의 경계가 흐려지는 것 같아요.
알림 클릭용 activity PendingIntent 는 알람을 수신한 PaymentReminderReceiver 에서 만들고 NotificationHelper 는 알림 UI, 채널만 담당하도록 만드는건 어떤가요?
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에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
리뷰에서는 알림 설정의 상태를 어디에서 관리하는 것이 적절한지 UI, ViewModel, 도메인 로직의 책임 분리가 자연스러운지 설정 변경이 실제 알림 동작에 반영되는 구조가 타당한지 를 중심으로 봐주시면 감사하겠습니다.🙇♀️
코드 리뷰 커뮤니케이션
현재는 선택 상태를 DB에 따로 저장하지 않아서, 계산이 필요한 시점마다 장바구니 전체 목록에서 선택된 ID에 해당하는 상품만 다시 걸러내고 있습니다. 이 방식이 괜찮은지, 아니면 선택 상태를 별도로 저장하는 편이 더 나은지 궁금합니다.
스크린샷