Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthrough데모 챌린지 기능을 전체 계층에 추가합니다: API 엔드포인트와 DTO, DTO↔엔티티 매핑, DefaultDemoRepository 및 DemoInterface, Fetch/Toggle/Advance 유스케이스와 DI 등록, ChallengeProgressViewModel 및 뷰 연동, Presentation DI·ViewFactory 수정. Changes
Sequence Diagram(s)sequenceDiagram
participant View as ChallengeProgressView
participant VM as ChallengeProgressViewModel
participant UC as UseCases
participant Repo as DefaultDemoRepository
participant API as DemoAPI
View->>VM: loadChallenge()
activate VM
VM->>UC: fetchChallengeUseCase.execute()
activate UC
UC->>Repo: fetchChallenges()
activate Repo
Repo->>API: HTTP GET /api/demo/challenges (DemoAPI.fetchChallenges(userID))
activate API
API-->>Repo: FetchChallengesResponseDTO
deactivate API
Repo->>Repo: DTO.toEntity() -> ChallengeEntity
Repo-->>UC: ChallengeEntity
deactivate Repo
UC-->>VM: ChallengeEntity
deactivate UC
VM-->>View: `@Published` 업데이트
View->>VM: toggleRoutine(routineID)
activate VM
VM->>UC: toggleRoutineUseCase.execute(routineID)
activate UC
UC->>Repo: toggleRoutine(routineID)
activate Repo
Repo->>API: PATCH /api/demo/challenges/routines/{routineID}/toggle
activate API
API-->>Repo: RoutineToggleResponseDTO
deactivate API
Repo->>Repo: DTO.toEntity() -> RoutineEntity
Repo-->>UC: RoutineEntity
deactivate Repo
UC-->>VM: RoutineEntity
deactivate UC
VM-->>View: `@Published` 업데이트
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
bff102d to
621d74c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/ChallengeProgressView.swift (2)
93-118: 로딩 및 에러 상태에 대한 UI 피드백이 없습니다.
viewModel에isLoading과errorMessage상태가 있지만, View에서 이를 표시하지 않습니다. 네트워크 요청 중 사용자에게 피드백이 없으면 UX가 저하될 수 있습니다.💡 로딩/에러 UI 추가 제안
var body: some View { ZStack { ScrollView { // 기존 코드... } if viewModel.isLoading { ProgressView() } } .alert("오류", isPresented: .constant(viewModel.errorMessage != nil)) { Button("확인") { // errorMessage 초기화 로직 } } message: { Text(viewModel.errorMessage ?? "") } .task { await viewModel.loadChallenge() } }
188-198: 버튼 상태가 항상.active입니다.
buttonState가 상수로 선언되어 있어 로딩 중이거나 모든 미션이 완료되지 않은 상태에서도 버튼이 활성화됩니다. 비즈니스 로직에 따라 조건부 상태 변경이 필요할 수 있습니다.♻️ 동적 버튼 상태 제안
- let buttonState: ButtonState = .active + private var buttonState: ButtonState { + if viewModel.isLoading { + return .loading // 또는 .inactive + } + return .active + }
🤖 Fix all issues with AI agents
In `@Cherrish-iOS/Cherrish-iOS/Data/Model/RoutineToggleResponseDTO.swift`:
- Around line 1-6: Update the file header comment to match the actual filename:
replace the incorrect "RoutineToggleRequestDTO.swift" on line 2 with
"RoutineToggleResponseDTO.swift" so the top-of-file comment aligns with the file
RoutineToggleResponseDTO.swift (verify any class/struct names like
RoutineToggleResponseDTO inside the file remain unchanged).
In `@Cherrish-iOS/Cherrish-iOS/Data/Repository/DemoRepository.swift`:
- Around line 22-47: The code currently falls back to userID = 1 in
fetchChallenges(), advance(), and toggleRoutine() by using
userDefaultService.load(key: .userID) ?? 1 which can cause accidental access to
other users' data; change these methods to explicitly handle a missing userID by
failing early: retrieve userID via userDefaultService.load(key: .userID), if nil
throw a clear domain error (e.g., create/throw a MissingUserError or use an
existing RepositoryError) instead of defaulting to 1, and then pass the
non-optional userID into DemoAPI.fetchChallenges, DemoAPI.advance, and
DemoAPI.routineToggle; update callers to handle the thrown error as needed.
In `@Cherrish-iOS/Cherrish-iOS/Domain/DomainDependencyAssembler.swift`:
- Around line 68-82: The current guard on DIContainer.shared.resolve(type:
DemoInterface.self) aborts the rest of DomainDependencyAssembler registrations
if the demo repository is missing; change it to optional let demoRepository =
DIContainer.shared.resolve(type: DemoInterface.self) and only use demoRepository
when registering Demo-dependent use cases (DefaultFetchChallengeUseCase,
DefaultToggleRoutineUseCase, DefaultAdvanceDayUseCase), while allowing the
subsequent registrations (e.g., FetchTreatmentsUseCase, FetchUserInfoUseCase and
any others) to proceed regardless; ensure each DIContainer.shared.register
closure captures demoRepository? safely (optional) so missing demo dependency no
longer blocks other registrations.
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/ChallengeProgressView.swift`:
- Around line 174-186: The CheckBoxComponent set closure creates a new Task for
every tap allowing duplicate toggle requests; update the UI/VM to prevent
concurrent toggles by adding a toggling state in the ViewModel (e.g.
togglingRoutineIDs Set) and checking it inside viewModel.toggleRoutine to
early-return if already toggling, or expose a per-routine isToggling published
property and bind it to disable the CheckBoxComponent while toggling; adjust the
Binding set to call viewModel.toggleRoutine only when not toggling (or let
toggleRoutine handle the guard) and ensure togglingRoutineIDs is
inserted/removed (or isToggling toggled) around the async work to prevent
duplicate requests for the same routineID.
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/Coordinator/ViewModel/ChallengeProgressViewModel.swift`:
- Around line 43-71: toggleRoutine currently only updates the local
todayRoutines array and never toggles isLoading, so aggregate fields
(progressPercentage, cherryLevel, remainingRoutinesToNextLevel) may become
stale; fix by setting isLoading = true at start and false in finally/after
catch, and update challengeData with server-calculated aggregates returned from
toggleRoutineUseCase.execute (preferable) or call the existing full refresh
method (e.g., loadChallenge/fetchChallenge) after the toggle to reload the full
ChallengeEntity; update references: toggleRoutine, toggleRoutineUseCase.execute,
challengeData, and isLoading accordingly.
| func fetchChallenges() async throws -> ChallengeEntity { | ||
| let userID: Int = userDefaultService.load(key: .userID) ?? 1 | ||
| let response = try await networkService.request( | ||
| DemoAPI.fetchChallenges(userID: userID), | ||
| decodingType: FetchChallengesResponseDTO.self | ||
| ) | ||
|
|
||
| return response.toEntity() | ||
| } | ||
|
|
||
| func advance() async throws -> ChallengeEntity { | ||
| let userID: Int = userDefaultService.load(key: .userID) ?? 1 | ||
| let response = try await networkService.request( | ||
| DemoAPI.advance(userID: userID), | ||
| decodingType: FetchChallengesResponseDTO.self | ||
| ) | ||
| return response.toEntity() | ||
| } | ||
|
|
||
| func toggleRoutine(routineID: Int) async throws -> RoutineEntity { | ||
| let userID: Int = userDefaultService.load(key: .userID) ?? 1 | ||
| let response = try await networkService.request( | ||
| DemoAPI.routineToggle(userID: userID, routineID: routineID), | ||
| decodingType: RoutineToggleResponseDTO.self | ||
| ) | ||
| return response.toEntity() |
There was a problem hiding this comment.
userID 기본값 1 사용은 위험합니다.
유저ID가 없을 때 임의 값으로 호출하면 타 사용자 데이터 조회/변경 가능성이 있습니다. 누락 시 에러로 처리하도록 변경하세요.
🔧 수정 제안
+enum DemoRepositoryError: Error {
+ case missingUserID
+}
+
struct DefaultDemoRepository: DemoInterface {
private let networkService: NetworkService
private let userDefaultService: UserDefaultService
@@
func fetchChallenges() async throws -> ChallengeEntity {
- let userID: Int = userDefaultService.load(key: .userID) ?? 1
+ guard let userID: Int = userDefaultService.load(key: .userID) else {
+ throw DemoRepositoryError.missingUserID
+ }
let response = try await networkService.request(
DemoAPI.fetchChallenges(userID: userID),
decodingType: FetchChallengesResponseDTO.self
)
return response.toEntity()
}
func advance() async throws -> ChallengeEntity {
- let userID: Int = userDefaultService.load(key: .userID) ?? 1
+ guard let userID: Int = userDefaultService.load(key: .userID) else {
+ throw DemoRepositoryError.missingUserID
+ }
let response = try await networkService.request(
DemoAPI.advance(userID: userID),
decodingType: FetchChallengesResponseDTO.self
)
return response.toEntity()
}
func toggleRoutine(routineID: Int) async throws -> RoutineEntity {
- let userID: Int = userDefaultService.load(key: .userID) ?? 1
+ guard let userID: Int = userDefaultService.load(key: .userID) else {
+ throw DemoRepositoryError.missingUserID
+ }
let response = try await networkService.request(
DemoAPI.routineToggle(userID: userID, routineID: routineID),
decodingType: RoutineToggleResponseDTO.self
)
return response.toEntity()
}
}🤖 Prompt for AI Agents
In `@Cherrish-iOS/Cherrish-iOS/Data/Repository/DemoRepository.swift` around lines
22 - 47, The code currently falls back to userID = 1 in fetchChallenges(),
advance(), and toggleRoutine() by using userDefaultService.load(key: .userID) ??
1 which can cause accidental access to other users' data; change these methods
to explicitly handle a missing userID by failing early: retrieve userID via
userDefaultService.load(key: .userID), if nil throw a clear domain error (e.g.,
create/throw a MissingUserError or use an existing RepositoryError) instead of
defaulting to 1, and then pass the non-optional userID into
DemoAPI.fetchChallenges, DemoAPI.advance, and DemoAPI.routineToggle; update
callers to handle the thrown error as needed.
| guard let demoRepository = DIContainer.shared.resolve(type: DemoInterface.self) else { | ||
| return | ||
| } | ||
|
|
||
| DIContainer.shared.register(type: FetchChallengeUseCase.self) { | ||
| return DefaultFetchChallengeUseCase(repository: demoRepository) | ||
| } | ||
|
|
||
| DIContainer.shared.register(type: ToggleRoutineUseCase.self) { | ||
| return DefaultToggleRoutineUseCase(repository: demoRepository) | ||
| } | ||
|
|
||
| DIContainer.shared.register(type: AdvanceDayUseCase.self) { | ||
| return DefaultAdvanceDayUseCase(repository: demoRepository) | ||
| } |
There was a problem hiding this comment.
DemoInterface 미해결 시 이후 DI 등록이 끊기는 문제
Line 68-70의 guard에서 실패하면 FetchTreatmentsUseCase, FetchUserInfoUseCase 등록까지 건너뛰게 됩니다. 데모 저장소 미등록이 전체 도메인 조립 실패로 이어질 수 있어 기능 영향이 큽니다. 데모 의존성은 옵션으로 처리하고 나머지 등록은 계속되도록 바꾸는 게 안전합니다.
✅ 제안 수정 (데모만 선택적으로 등록)
- guard let demoRepository = DIContainer.shared.resolve(type: DemoInterface.self) else {
- return
- }
-
- DIContainer.shared.register(type: FetchChallengeUseCase.self) {
- return DefaultFetchChallengeUseCase(repository: demoRepository)
- }
-
- DIContainer.shared.register(type: ToggleRoutineUseCase.self) {
- return DefaultToggleRoutineUseCase(repository: demoRepository)
- }
-
- DIContainer.shared.register(type: AdvanceDayUseCase.self) {
- return DefaultAdvanceDayUseCase(repository: demoRepository)
- }
+ if let demoRepository = DIContainer.shared.resolve(type: DemoInterface.self) {
+ DIContainer.shared.register(type: FetchChallengeUseCase.self) {
+ return DefaultFetchChallengeUseCase(repository: demoRepository)
+ }
+
+ DIContainer.shared.register(type: ToggleRoutineUseCase.self) {
+ return DefaultToggleRoutineUseCase(repository: demoRepository)
+ }
+
+ DIContainer.shared.register(type: AdvanceDayUseCase.self) {
+ return DefaultAdvanceDayUseCase(repository: demoRepository)
+ }
+ }🤖 Prompt for AI Agents
In `@Cherrish-iOS/Cherrish-iOS/Domain/DomainDependencyAssembler.swift` around
lines 68 - 82, The current guard on DIContainer.shared.resolve(type:
DemoInterface.self) aborts the rest of DomainDependencyAssembler registrations
if the demo repository is missing; change it to optional let demoRepository =
DIContainer.shared.resolve(type: DemoInterface.self) and only use demoRepository
when registering Demo-dependent use cases (DefaultFetchChallengeUseCase,
DefaultToggleRoutineUseCase, DefaultAdvanceDayUseCase), while allowing the
subsequent registrations (e.g., FetchTreatmentsUseCase, FetchUserInfoUseCase and
any others) to proceed regardless; ensure each DIContainer.shared.register
closure captures demoRepository? safely (optional) so missing demo dependency no
longer blocks other registrations.
| ForEach(todayRoutines, id: \.routineID) { routine in | ||
| CheckBoxComponent( | ||
| text: missions[index], | ||
| isChecked: $selectedStates[index] | ||
| text: routine.name, | ||
| isChecked: Binding( | ||
| get: { routine.isComplete }, | ||
| set: { _ in | ||
| Task { | ||
| await viewModel.toggleRoutine(routineID: routine.routineID) | ||
| } | ||
| } | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
빠른 연속 탭 시 중복 토글 요청이 발생할 수 있습니다.
CheckBoxComponent의 set 클로저에서 매번 새로운 Task를 생성하므로, 사용자가 빠르게 여러 번 탭하면 동일한 루틴에 대해 여러 토글 요청이 동시에 발생할 수 있습니다. 이로 인해 최종 상태가 예상과 다를 수 있습니다.
🔧 중복 요청 방지를 위한 제안
ViewModel에 진행 중인 루틴 ID를 추적하거나, 토글 중에는 해당 체크박스를 비활성화하는 방법을 고려해 주세요:
// ViewModel에 추가
`@Published` private(set) var togglingRoutineIDs: Set<Int> = []
`@MainActor`
func toggleRoutine(routineID: Int) async {
guard !togglingRoutineIDs.contains(routineID) else { return }
togglingRoutineIDs.insert(routineID)
defer { togglingRoutineIDs.remove(routineID) }
// 기존 로직...
}🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/ChallengeProgressView.swift`
around lines 174 - 186, The CheckBoxComponent set closure creates a new Task for
every tap allowing duplicate toggle requests; update the UI/VM to prevent
concurrent toggles by adding a toggling state in the ViewModel (e.g.
togglingRoutineIDs Set) and checking it inside viewModel.toggleRoutine to
early-return if already toggling, or expose a per-routine isToggling published
property and bind it to disable the CheckBoxComponent while toggling; adjust the
Binding set to call viewModel.toggleRoutine only when not toggling (or let
toggleRoutine handle the guard) and ensure togglingRoutineIDs is
inserted/removed (or isToggling toggled) around the async work to prevent
duplicate requests for the same routineID.
| @MainActor | ||
| func toggleRoutine(routineID: Int) async { | ||
| do { | ||
| let updatedRoutine = try await toggleRoutineUseCase.execute(routineID: routineID) | ||
|
|
||
| guard let currentData = challengeData else { return } | ||
|
|
||
| let updatedRoutines = currentData.todayRoutines.map { routine in | ||
| if routine.routineID == updatedRoutine.routineID { | ||
| return updatedRoutine | ||
| } | ||
| return routine | ||
| } | ||
|
|
||
| challengeData = ChallengeEntity( | ||
| challengeID: currentData.challengeID, | ||
| title: currentData.title, | ||
| currentDay: currentData.currentDay, | ||
| progressPercentage: currentData.progressPercentage, | ||
| cherryLevel: currentData.cherryLevel, | ||
| cherryLevelName: currentData.cherryLevelName, | ||
| progressToNextLevel: currentData.progressToNextLevel, | ||
| remainingRoutinesToNextLevel: currentData.remainingRoutinesToNextLevel, | ||
| todayRoutines: updatedRoutines | ||
| ) | ||
| } catch { | ||
| errorMessage = error.localizedDescription | ||
| } | ||
| } |
There was a problem hiding this comment.
루틴 토글 후 집계 데이터가 갱신되지 않을 수 있습니다.
toggleRoutine()에서 로컬 todayRoutines 배열만 업데이트하고 있어, 서버에서 계산되는 progressPercentage, cherryLevel, remainingRoutinesToNextLevel 등의 집계 값이 갱신되지 않습니다. 루틴 완료 시 이러한 값들이 변경되어야 한다면 데이터 불일치가 발생할 수 있습니다.
또한, 다른 메서드들과 달리 isLoading 상태를 설정하지 않아 UI에서 로딩 표시가 되지 않습니다.
🔧 집계 데이터 갱신을 위한 제안
옵션 1: 토글 후 전체 데이터 다시 로드
`@MainActor`
func toggleRoutine(routineID: Int) async {
+ isLoading = true
do {
- let updatedRoutine = try await toggleRoutineUseCase.execute(routineID: routineID)
-
- guard let currentData = challengeData else { return }
-
- let updatedRoutines = currentData.todayRoutines.map { routine in
- if routine.routineID == updatedRoutine.routineID {
- return updatedRoutine
- }
- return routine
- }
-
- challengeData = ChallengeEntity(...)
+ _ = try await toggleRoutineUseCase.execute(routineID: routineID)
+ challengeData = try await fetchChallengeUseCase.execute()
} catch {
errorMessage = error.localizedDescription
}
+ isLoading = false
}옵션 2: 토글 API가 업데이트된 ChallengeEntity를 반환하도록 수정
🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/Coordinator/ViewModel/ChallengeProgressViewModel.swift`
around lines 43 - 71, toggleRoutine currently only updates the local
todayRoutines array and never toggles isLoading, so aggregate fields
(progressPercentage, cherryLevel, remainingRoutinesToNextLevel) may become
stale; fix by setting isLoading = true at start and false in finally/after
catch, and update challengeData with server-calculated aggregates returned from
toggleRoutineUseCase.execute (preferable) or call the existing full refresh
method (e.g., loadChallenge/fetchChallenge) after the toggle to reload the full
ChallengeEntity; update references: toggleRoutine, toggleRoutineUseCase.execute,
challengeData, and isLoading accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/Coordinator/ViewModel/ChallengeProgressViewModel.swift`:
- Around line 80-93: The advanceDay() method currently calls updateInfo()
regardless of success; move the call into the success path so updateInfo() runs
only after challengeData is successfully updated. Specifically, inside
advanceDay() call updateInfo() immediately after assigning challengeData = try
await advanceDayUseCase.execute() (within the do block) and keep isLoading and
errorMessage handling as-is so failures set errorMessage and skip updateInfo().
♻️ Duplicate comments (2)
Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/Coordinator/ViewModel/ChallengeProgressViewModel.swift (1)
49-78: 루틴 토글 후 집계 데이터가 갱신되지 않을 수 있습니다.이 문제는 이전 리뷰에서 이미 지적되었습니다.
toggleRoutine()에서 로컬todayRoutines배열만 업데이트하고 서버에서 계산되는progressPercentage,cherryLevel,remainingRoutinesToNextLevel등의 집계 값은 갱신되지 않습니다. 또한isLoading상태도 설정하지 않습니다.Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/ChallengeProgressView.swift (1)
136-148: 빠른 연속 탭 시 중복 토글 요청이 발생할 수 있습니다.이 문제는 이전 리뷰에서 이미 지적되었습니다. 사용자가 체크박스를 빠르게 여러 번 탭하면 동일한 루틴에 대해 여러 토글 요청이 동시에 발생하여 최종 상태가 예상과 다를 수 있습니다. ViewModel에서 진행 중인 루틴 ID를 추적하는 방식을 고려해 주세요.
| @MainActor | ||
| func advanceDay() async { | ||
| isLoading = true | ||
| errorMessage = nil | ||
|
|
||
| do { | ||
| challengeData = try await advanceDayUseCase.execute() | ||
| } catch { | ||
| errorMessage = error.localizedDescription | ||
| } | ||
|
|
||
| isLoading = false | ||
| updateInfo() | ||
| } |
There was a problem hiding this comment.
에러 발생 시에도 updateInfo()가 호출됩니다.
updateInfo()가 do-catch 블록 외부에서 호출되어, API 호출이 실패해도 실행됩니다. 에러 발생 시 이전 challengeData가 남아있다면 UI가 갱신되어 사용자에게 혼란을 줄 수 있습니다.
🔧 제안된 수정
`@MainActor`
func advanceDay() async {
isLoading = true
errorMessage = nil
do {
challengeData = try await advanceDayUseCase.execute()
+ updateInfo()
} catch {
errorMessage = error.localizedDescription
}
isLoading = false
- updateInfo()
}🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/Coordinator/ViewModel/ChallengeProgressViewModel.swift`
around lines 80 - 93, The advanceDay() method currently calls updateInfo()
regardless of success; move the call into the success path so updateInfo() runs
only after challengeData is successfully updated. Specifically, inside
advanceDay() call updateInfo() immediately after assigning challengeData = try
await advanceDayUseCase.execute() (within the do block) and keep isLoading and
errorMessage handling as-is so failures set errorMessage and skip updateInfo().

🔗 연결된 이슈
📄 작업 내용
💻 주요 코드 설명
기존 연결이랑 비슷해서 코드 읽어보시면 될 거 같아요