Conversation
Walkthrough네트워크 오류 분류 및 재시도 인프라를 도입합니다. NetworkService에서 URLError를 매핑하고, 리포지토리에서 NetworkError를 DomainError로 변환하며, Presentation 계층에 NetworkRetryHandler와 NetworkErrorView를 연결해 재시도 흐름을 노출합니다. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant VC as ViewController
participant VM as ViewModel
participant Retry as NetworkRetryHandler
participant Repo as Repository
participant Net as NetworkService
participant API as Remote API
User->>VC: 사용자 요청(예: 새로고침)
VC->>VM: ViewModel에 요청 전달
VM->>Repo: 데이터 요청 호출
Repo->>Net: networkService.request
Net->>API: HTTP 요청 전송
alt 네트워크 실패
API-->>Net: 오류 응답
Net-->>Repo: NetworkError로 매핑
Repo-->>VM: throw DomainError(requireRetry/business/unknown)
VM->>Retry: handleNetworkError(error, retryAction)
Retry-->>VM: networkErrorActionSubject에 retryAction 발행
VM-->>VC: Output.networkErrorPublisher 통해 재시도 콜백 전달
VC->>VC: BaseViewController가 NetworkErrorView 표시
User->>VC: 재시도 버튼 탭
VC->>VM: retryAction 실행 (요청 재시작)
else 성공
API-->>Net: 성공 응답
Net-->>Repo: 데이터 반환
Repo-->>VM: 성공 결과 반환
VM->>Retry: clearRetryState()
VM-->>VC: 정상 데이터로 UI 갱신
VC->>VC: NetworkErrorView 숨김
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Projects/Presentation/Sources/Report/View/ReportCompleteViewController.swift (1)
83-88:⚠️ Potential issue | 🟠 Major
configureAttribute()와configureLayout()이 중복 호출됩니다.
super.viewDidLoad()가BaseViewController에서 이미configureAttribute(),configureLayout(),bind()를 호출합니다. 따라서 Line 85-86에서 다시 호출하면 메서드가 두 번 실행되어,addSubview/addArrangedSubview등이 중복 수행됩니다.🐛 중복 호출 제거 제안
override func viewDidLoad() { super.viewDidLoad() - configureAttribute() - configureLayout() viewModel.action(input: .fetchReportDetail(reportId: reportId)) }Based on learnings:
BaseViewController의 viewDidLoad() 메서드에서 이미 configureAttribute(), configureLayout(), bind()를 호출하므로, 하위 클래스에서 super.viewDidLoad()를 호출하면 이 메서드들이 자동으로 호출된다. 따라서 하위 클래스에서 추가로 호출할 필요가 없다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Report/View/ReportCompleteViewController.swift` around lines 83 - 88, BaseViewController.viewDidLoad() already calls configureAttribute(), configureLayout(), and bind(), so remove the duplicate calls in ReportCompleteViewController.viewDidLoad(): keep super.viewDidLoad() and the viewModel.action(input: .fetchReportDetail(reportId: reportId)) line, but delete the explicit configureAttribute() and configureLayout() invocations to avoid double addSubview/addArrangedSubview side effects.Projects/Presentation/Sources/RoutineCreation/View/RoutineCreationViewController.swift (1)
239-317:⚠️ Potential issue | 🟠 Major
bind()메서드 마지막에bindNetworkError호출 추가 필요
RoutineCreationViewModel은networkErrorPublisher를 노출하고 있으나,bind()메서드에서 이를 바인딩하지 않고 있습니다.RoutineListViewController,ReportHistoryViewController,ResultRecommendedRoutineViewController등 다른 뷰 컨트롤러에서는 일관되게bindNetworkError(from: viewModel.output.networkErrorPublisher)를 호출하고 있으므로, 동일한 패턴을 적용해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/RoutineCreation/View/RoutineCreationViewController.swift` around lines 239 - 317, The bind() method in RoutineCreationViewController is missing the network error binding — call bindNetworkError(from: viewModel.output.networkErrorPublisher) at the end of bind() so the controller subscribes to RoutineCreationViewModel's networkErrorPublisher; update the RoutineCreationViewController.bind() method to invoke bindNetworkError(from: viewModel.output.networkErrorPublisher) after the existing publishers are stored to ensure consistent network-error handling.Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift (1)
126-156:⚠️ Potential issue | 🟠 Major여러 독립 비동기 작업이 하나의
networkRetryHandler를 공유하면 race condition이 발생할 수 있습니다.HomeViewModel에는
loadNickname,fetchEmotion,fetchRoutines,updateRoutineCompletion,checkVersion등 여러 독립 비동기 작업이 있으며, 모두 동일한networkRetryHandler를 사용합니다.발생 가능한 시나리오:
fetchRoutines실패 → 에러 뷰 표시 (retry = fetchRoutines)loadNickname성공 →clearRetryState()호출 → 에러 뷰 숨김- 사용자는 fetchRoutines 실패를 재시도할 기회를 잃음
또는 반대로:
loadNickname실패 → retry = loadNicknamefetchRoutines실패 → retry가 fetchRoutines로 덮어씌워짐- 사용자가 retry → loadNickname은 재시도되지 않음
viewWillAppear에서 여러 작업이 동시에 트리거되므로, 하나의 작업이 성공하면 다른 실패 작업의 retry 상태가 소실됩니다. 단기적으로는 retry action에서 모든 초기 작업을 다시 실행하는 방식을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift` around lines 126 - 156, Multiple concurrent async tasks (loadNickname, fetchEmotion, fetchRoutines, updateRoutineCompletion, checkVersion) share a single networkRetryHandler causing races where one task's success clears another's retry state; fix by giving each independent operation its own retry state or by making networkRetryHandler support per-operation IDs: either add separate handlers (e.g., nicknameRetryHandler, emotionRetryHandler) and replace networkRetryHandler calls in loadNickname/fetchEmotion/etc. with the corresponding handler, or change networkRetryHandler.handleNetworkError/clearRetryState to accept an operation identifier and only clear or retry that specific operation; also update viewWillAppear to trigger the correct per-operation retries (or a centralized retry that re-runs all initial tasks) instead of relying on a single global retry state.Projects/Presentation/Sources/RoutineCreation/ViewModel/RoutineCreationViewModel.swift (1)
128-171:⚠️ Potential issue | 🔴 Critical
fetchRoutine과fetchRecommendedRoutine성공 시clearRetryState()호출이 누락되었습니다.
registerRoutine에서는 성공 시networkRetryHandler.clearRetryState()를 호출하지만,fetchRoutine(Line 128-171)과fetchRecommendedRoutine(Line 174-191)에서는 호출하지 않습니다. 네트워크 에러 뷰가 표시된 상태에서 재시도가 성공하면 에러 뷰가 사라지지 않을 수 있습니다.🐛 수정 제안 - fetchRoutine
routineId = id updateIsRoutineValid() + + networkRetryHandler.clearRetryState() } catch {🐛 수정 제안 - fetchRecommendedRoutine
routineType = routine.type updateIsRoutineValid() + + networkRetryHandler.clearRetryState() } catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/RoutineCreation/ViewModel/RoutineCreationViewModel.swift` around lines 128 - 171, fetchRoutine and fetchRecommendedRoutine fail to clear the network retry/error UI on a successful response; after the async fetch completes successfully (inside fetchRoutine after you set periodStart/periodEnd/name/subRoutines/repeatType/executionTime/routineId and before updateIsRoutineValid or returning) call networkRetryHandler.clearRetryState(), and likewise add the same call in fetchRecommendedRoutine immediately after successfully processing the fetched recommended routine (i.e., after updating any subjects/state and before any return or UI-update call) so a prior network error view is dismissed on success.
🧹 Nitpick comments (7)
Projects/DataSource/Sources/NetworkService/NetworkService.swift (1)
63-75:URLError처리 로직이 적절합니다.연결 관련 에러를
needRetry로 매핑하고, 나머지를unknown으로 처리하는 구조가 깔끔합니다.한 가지 고려사항:
.dataNotAllowed(셀룰러 데이터 비활성화)도 연결 관련 에러로needRetry에 포함시킬 수 있습니다.선택적 개선안
- if [.notConnectedToInternet, .timedOut, .networkConnectionLost].contains(error.code) { + if [.notConnectedToInternet, .timedOut, .networkConnectionLost, .dataNotAllowed].contains(error.code) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/DataSource/Sources/NetworkService/NetworkService.swift` around lines 63 - 75, Add .dataNotAllowed to the set of URLError codes treated as transient so the existing URLError catch maps cellular-data-disabled cases to NetworkError.needRetry; update the check in the catch-let-URLError block that currently tests [.notConnectedToInternet, .timedOut, .networkConnectionLost] to also include .dataNotAllowed (refer to the URLError handling around URLSession.shared.data(for: request) in NetworkService.swift).Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift (1)
43-44: 배경색이.white로 하드코딩되어 있습니다.다른 뷰에서는
.systemBackground나BitnagilColor상수를 사용하는 반면, 여기서는.white로 하드코딩되어 있습니다. 다크 모드를 지원하게 될 경우 문제가 될 수 있으므로, 프로젝트 컨벤션에 맞게.systemBackground또는 적절한BitnagilColor사용을 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift` around lines 43 - 44, In NetworkErrorView replace hardcoded .white assignments (backgroundColor and contentView.backgroundColor) with the project's dynamic color choice (e.g. .systemBackground or the appropriate BitnagilColor constant) so the view follows dark mode and project conventions; update the assignments in the NetworkErrorView setup/initializer where backgroundColor and contentView.backgroundColor are set to use the chosen color.Projects/DataSource/Sources/Repository/ReportRepository.swift (1)
37-50: 에러 매핑 로직의 반복 — 헬퍼 함수 추출을 고려해 주세요.
NetworkError→DomainError변환 로직이 이 파일의 세 메서드뿐 아니라FileRepository,LocationRepository,OnboardingRepository,RecommendedRoutineRepository등 모든 리포지토리에서 동일하게 반복됩니다. 공통 헬퍼 함수로 추출하면 유지보수성이 향상됩니다.♻️ 에러 매핑 헬퍼 함수 제안
예를 들어,
NetworkErrorextension 또는 별도 유틸리티로:extension NetworkError { func toDomainError() -> DomainError { switch self { case .needRetry, .invalidURL, .emptyData: return .requireRetry default: return .business(self.description) } } }각 리포지토리에서는 다음과 같이 사용:
- } catch let error as NetworkError { - switch error { - case .needRetry, .invalidURL, .emptyData: - throw DomainError.requireRetry - default: - throw DomainError.business(error.description) - } - } catch { - throw DomainError.unknown - } + } catch let error as NetworkError { + throw error.toDomainError() + } catch { + throw DomainError.unknown + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/DataSource/Sources/Repository/ReportRepository.swift` around lines 37 - 50, Extract the repeated NetworkError→DomainError mapping into a shared helper (e.g., add a NetworkError extension method like toDomainError() or a static util function) and replace the switch in ReportRepository's do-catch (the catch for "let error as NetworkError") with throw error.toDomainError() (or throw NetworkErrorMapper.map(error)). Update other repositories (FileRepository, LocationRepository, OnboardingRepository, RecommendedRoutineRepository) to call the same helper so all occurrences use the single implementation that returns .requireRetry for .needRetry/.invalidURL/.emptyData and .business(error.description) for others.Projects/Presentation/Sources/Home/View/HomeViewController.swift (1)
263-265:dimmedView가networkErrorView위에 추가되어 네트워크 에러 뷰를 가릴 수 있습니다.
super.configureLayout()(Line 226)에서networkErrorView가 먼저 추가된 후, Line 263에서dimmedView, Line 264에서floatingMenu, Line 265에서floatingButton이 추가됩니다.handleNetworkErrorView에서bringSubviewToFront를 호출하므로 표시 시점에는 문제가 없지만, floating 메뉴가 열린 상태에서 네트워크 에러가 발생하면 dimmedView와의 z-order 경쟁이 발생할 수 있습니다. 에러 표시 시 floating 메뉴/dimmedView를 닫는 것을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Home/View/HomeViewController.swift` around lines 263 - 265, The dimmedView/floatingMenu/floatingButton are added after super.configureLayout() which added networkErrorView, causing z-order conflicts when a network error occurs while the floating menu is open; update handleNetworkErrorView to first close or hide the floating menu and dimmedView (e.g., call floatingMenu.dismiss()/close() or set floatingMenu.isHidden = true and set dimmedView.isHidden = true/removeFromSuperview()) before presenting or bringing networkErrorView to front, or alternatively ensure you call view.bringSubviewToFront(networkErrorView) and then hide/remove dimmedView so the networkErrorView is never obscured.Projects/Presentation/Sources/Common/Protocol/BaseViewController.swift (1)
47-53:networkErrorView가 서브클래스의 뷰보다 먼저 추가되어 뷰 계층 뒤에 위치합니다.
super.configureLayout()이 먼저 호출되므로networkErrorView는 서브클래스에서 추가하는 뷰들 아래에 위치하게 됩니다.handleNetworkErrorView에서bringSubviewToFront를 호출하여 이를 처리하고 있으므로 동작은 정상이지만, 서브클래스에서super.configureLayout()을 마지막에 호출하면 의도한 대로 networkErrorView가 항상 최상위에 위치할 수 있습니다. 현재 구현도 동작하므로 참고 사항입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Common/Protocol/BaseViewController.swift` around lines 47 - 53, The networkErrorView is being added in BaseViewController.configureLayout before subclass views when subclasses call super.configureLayout() first, causing it to sit behind subclass content; update the layout flow so networkErrorView is always topmost by either (A) moving the networkErrorView addition and its constraints out of configureLayout into a point after subclass layout (e.g., require subclasses to call super.configureLayout() last) or (B) keep it in configureLayout but immediately call bringSubviewToFront(networkErrorView) (or call handleNetworkErrorView) at the end of configureLayout to ensure networkErrorView is brought above any subclass-added views; reference networkErrorView, configureLayout, handleNetworkErrorView, bringSubviewToFront and super.configureLayout() when making the change.Projects/DataSource/Sources/Repository/RoutineRepository.swift (1)
27-38: 에러 매핑 로직이 7개 메서드에서 동일하게 반복됩니다. 헬퍼로 추출하세요.모든 메서드에서 동일한
do-catch+switch패턴이 반복됩니다. 유지보수성과 일관성을 위해 공통 헬퍼를 추출하는 것을 권장합니다.♻️ 에러 변환 헬퍼 추출 예시
private func mapToDomainError(_ error: Error) -> DomainError { if let networkError = error as? NetworkError { switch networkError { case .needRetry: return .requireRetry default: return .business(networkError.description) } } return .unknown }사용 예시:
- do { - _ = try await networkService.request(endpoint: endpoint, type: EmptyResponseDTO.self) - } catch let error as NetworkError { - switch error { - case .needRetry, .invalidURL, .emptyData: - throw DomainError.requireRetry - default: - throw DomainError.business(error.description) - } - } catch { - throw DomainError.unknown - } + do { + _ = try await networkService.request(endpoint: endpoint, type: EmptyResponseDTO.self) + } catch { + throw mapToDomainError(error) + }Also applies to: 44-57, 63-83, 99-110, 116-127, 133-144, 156-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/DataSource/Sources/Repository/RoutineRepository.swift` around lines 27 - 38, Extract a shared helper in RoutineRepository named something like mapToDomainError(_:) that takes Error and returns DomainError using the same mapping used in the duplicated do-catch blocks (cast to NetworkError and switch: .needRetry, .invalidURL, .emptyData -> .requireRetry; default NetworkError -> .business(networkError.description); non-NetworkError -> .unknown), then replace the repeated catch logic in each method (the seven methods that currently catch NetworkError and map to DomainError) to call this helper and rethrow the resulting DomainError.Projects/Presentation/Sources/Report/ViewModel/ReportHistoryViewModel.swift (1)
168-174:fetchReports재시도 로직은 올바르지만, 네트워크 외 에러는 무시됩니다.
handleNetworkError는DomainError.requireRetry인 경우에만 동작하므로,.business나.unknown에러 발생 시 사용자에게 아무런 피드백이 없습니다. 기존 TODO 주석(Line 171)이 이를 인지하고 있지만, 추후 처리가 필요합니다.비네트워크 에러에 대한 사용자 피드백(토스트, 알림 등) 처리 구현을 도와드릴까요? 또는 추적을 위한 이슈를 생성해드릴까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Report/ViewModel/ReportHistoryViewModel.swift` around lines 168 - 174, The catch currently only passes errors to networkRetryHandler.handleNetworkError (which handles DomainError.requireRetry) so non-network errors (.business, .unknown) are ignored; update the catch in fetchReports to distinguish DomainError.requireRetry vs other errors by inspecting the thrown error, call networkRetryHandler.handleNetworkError for retryable DomainError cases, and for .business/.unknown paths invoke a user-feedback mechanism (e.g., showToast/showAlert/logError) and keep the existing TODO comment removed; reference networkRetryHandler.handleNetworkError, fetchReports, and DomainError.requireRetry/.business/.unknown when adding the conditional handling and feedback call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Projects/DataSource/Sources/Repository/EmotionRepository.swift`:
- Around line 37-52: In loadEmotion, don’t throw a NetworkError when the
response is nil then immediately catch and map it — throw the appropriate
DomainError directly instead; replace the guard’s throw
NetworkError.unknown(...) with throw DomainError.business("Emotion Response를
받아오지 못했습니다.") (fixing the "Reponse" -> "Response" typo), so the downstream catch
for NetworkError in the same method no longer needs to translate this case.
In `@Projects/DataSource/Sources/Repository/OnboardingRepository.swift`:
- Around line 17-30: The method loadOnboardingResult currently only catches
NetworkError (catch let error as NetworkError) so non-network errors like
UserError.onboardingLoadFailed will escape; to align with other repository
methods (e.g., registerOnboarding, registerRecommendedRoutines) add a general
catch after the NetworkError catch that throws DomainError.unknown, or
alternatively move the throw UserError.onboardingLoadFailed outside the do so
it’s handled consistently; update loadOnboardingResult accordingly to ensure all
non-NetworkError exceptions are mapped to DomainError.unknown.
In `@Projects/DataSource/Sources/Repository/ReportRepository.swift`:
- Around line 82-96: The DTO conversion error from response.toReportEntity() is
being caught as NetworkError and mapped to DomainError.business; separate
network errors from DTO-conversion errors by isolating the network call and the
DTO conversion into two try/catch scopes: call networkService.request(...)
inside its own do/catch that handles NetworkError (switching
needRetry/invalidURL/emptyData -> DomainError.requireRetry, others ->
DomainError.business), then after that, perform response.toReportEntity() in a
separate try and map its failures to the appropriate DomainError (e.g.,
DomainError.unknown or a new DomainError.decode) instead of letting its
NetworkError.decodingError fall into the network catch; reference
networkService.request, response.toReportEntity(), NetworkError, DomainError,
and ReportDTO.swift when making the change.
In `@Projects/DataSource/Sources/Repository/RoutineRepository.swift`:
- Around line 29-38: The catch block in RoutineRepository currently maps
NetworkError.invalidURL and .emptyData to DomainError.requireRetry; change it so
only .needRetry maps to DomainError.requireRetry, while .invalidURL and
.emptyData are mapped to non-retry errors (e.g.,
DomainError.business(error.description) or DomainError.unknown as appropriate).
Update the catch handling for "catch let error as NetworkError" to switch
.needRetry -> DomainError.requireRetry and handle .invalidURL and .emptyData by
throwing a business/unknown domain error, leaving other cases to the default
branch that throws DomainError.business(error.description).
In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift`:
- Around line 60-61: retryButton currently sets both retryButton.tintColor and
retryButton.backgroundColor to BitnagilColor.gray10 which will hide any icon or
system image; inspect the NetworkErrorView (or the retryButton setup) and either
remove the tintColor assignment if you want the default system tint, or change
retryButton.tintColor to a contrasting color (e.g., BitnagilColor.white or a
semantic accent color) so icons remain visible while leaving
retryButton.backgroundColor as BitnagilColor.gray10.
- Line 16: The enum contains a camelCase mismatch: rename the constant
boldtitleLabelTopSpacing to boldTitleLabelTopSpacing so it matches
boldTitleLabelHeight, and update all usages (including the reference near the
other usage around the former Line 98) to the new name; ensure you only change
the identifier (keep the CGFloat value) in the enum and replace every occurrence
of boldtitleLabelTopSpacing with boldTitleLabelTopSpacing in
NetworkErrorView-related code.
In `@Projects/Presentation/Sources/Common/Protocol/BaseViewController.swift`:
- Around line 22-26: The deinit unconditionally unhides the tab bar causing
visual glitches; add a Bool property (e.g. didShowNetworkErrorView or
isNetworkErrorViewVisible) on BaseViewController, set it to true inside
handleNetworkErrorView when you present the network error UI, and in deinit only
perform DispatchQueue.main.async { [weak tabBarController =
self.tabBarController] in if self.didShowNetworkErrorView {
tabBarController?.tabBar.isHidden = false } } so the tab bar is restored only if
the network error view was actually shown (ensure the flag is updated on the
main thread).
In
`@Projects/Presentation/Sources/ResultRecommendedRoutine/ViewModel/ResultRecommendedRoutineViewModel.swift`:
- Around line 169-172: The retry handler is wired to call
fetchResultRecommendedRoutines() on network error but should reattempt the
original registration; update the closure passed to
networkRetryHandler.handleNetworkError(error) so it calls
registerRecommendedRoutine() (not fetchResultRecommendedRoutines()) from the
catch block in registerRecommendedRoutine(), preserving [weak self] capture and
context so the registration is retried correctly.
In
`@Projects/Presentation/Sources/RoutineList/ViewModel/RoutineListViewModel.swift`:
- Around line 139-142: The retry handler is wired to call fetchRoutines() on
failure but should re-attempt the original delete; update the closure passed to
networkRetryHandler.handleNetworkError to call deleteRoutine with the same
identifying parameter(s) used in the original deletion (e.g.
self?.deleteRoutine(id: routineID) or self?.deleteRoutine(routine: routine)),
ensuring you capture any needed routineID/routine into a local constant before
the async call and keep [weak self] in the closure to avoid retain cycles;
replace the current self?.fetchRoutines() invocation with that deleteRoutine
retry call.
---
Outside diff comments:
In `@Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift`:
- Around line 126-156: Multiple concurrent async tasks (loadNickname,
fetchEmotion, fetchRoutines, updateRoutineCompletion, checkVersion) share a
single networkRetryHandler causing races where one task's success clears
another's retry state; fix by giving each independent operation its own retry
state or by making networkRetryHandler support per-operation IDs: either add
separate handlers (e.g., nicknameRetryHandler, emotionRetryHandler) and replace
networkRetryHandler calls in loadNickname/fetchEmotion/etc. with the
corresponding handler, or change
networkRetryHandler.handleNetworkError/clearRetryState to accept an operation
identifier and only clear or retry that specific operation; also update
viewWillAppear to trigger the correct per-operation retries (or a centralized
retry that re-runs all initial tasks) instead of relying on a single global
retry state.
In
`@Projects/Presentation/Sources/Report/View/ReportCompleteViewController.swift`:
- Around line 83-88: BaseViewController.viewDidLoad() already calls
configureAttribute(), configureLayout(), and bind(), so remove the duplicate
calls in ReportCompleteViewController.viewDidLoad(): keep super.viewDidLoad()
and the viewModel.action(input: .fetchReportDetail(reportId: reportId)) line,
but delete the explicit configureAttribute() and configureLayout() invocations
to avoid double addSubview/addArrangedSubview side effects.
In
`@Projects/Presentation/Sources/RoutineCreation/View/RoutineCreationViewController.swift`:
- Around line 239-317: The bind() method in RoutineCreationViewController is
missing the network error binding — call bindNetworkError(from:
viewModel.output.networkErrorPublisher) at the end of bind() so the controller
subscribes to RoutineCreationViewModel's networkErrorPublisher; update the
RoutineCreationViewController.bind() method to invoke bindNetworkError(from:
viewModel.output.networkErrorPublisher) after the existing publishers are stored
to ensure consistent network-error handling.
In
`@Projects/Presentation/Sources/RoutineCreation/ViewModel/RoutineCreationViewModel.swift`:
- Around line 128-171: fetchRoutine and fetchRecommendedRoutine fail to clear
the network retry/error UI on a successful response; after the async fetch
completes successfully (inside fetchRoutine after you set
periodStart/periodEnd/name/subRoutines/repeatType/executionTime/routineId and
before updateIsRoutineValid or returning) call
networkRetryHandler.clearRetryState(), and likewise add the same call in
fetchRecommendedRoutine immediately after successfully processing the fetched
recommended routine (i.e., after updating any subjects/state and before any
return or UI-update call) so a prior network error view is dismissed on success.
---
Nitpick comments:
In `@Projects/DataSource/Sources/NetworkService/NetworkService.swift`:
- Around line 63-75: Add .dataNotAllowed to the set of URLError codes treated as
transient so the existing URLError catch maps cellular-data-disabled cases to
NetworkError.needRetry; update the check in the catch-let-URLError block that
currently tests [.notConnectedToInternet, .timedOut, .networkConnectionLost] to
also include .dataNotAllowed (refer to the URLError handling around
URLSession.shared.data(for: request) in NetworkService.swift).
In `@Projects/DataSource/Sources/Repository/ReportRepository.swift`:
- Around line 37-50: Extract the repeated NetworkError→DomainError mapping into
a shared helper (e.g., add a NetworkError extension method like toDomainError()
or a static util function) and replace the switch in ReportRepository's do-catch
(the catch for "let error as NetworkError") with throw error.toDomainError() (or
throw NetworkErrorMapper.map(error)). Update other repositories (FileRepository,
LocationRepository, OnboardingRepository, RecommendedRoutineRepository) to call
the same helper so all occurrences use the single implementation that returns
.requireRetry for .needRetry/.invalidURL/.emptyData and
.business(error.description) for others.
In `@Projects/DataSource/Sources/Repository/RoutineRepository.swift`:
- Around line 27-38: Extract a shared helper in RoutineRepository named
something like mapToDomainError(_:) that takes Error and returns DomainError
using the same mapping used in the duplicated do-catch blocks (cast to
NetworkError and switch: .needRetry, .invalidURL, .emptyData -> .requireRetry;
default NetworkError -> .business(networkError.description); non-NetworkError ->
.unknown), then replace the repeated catch logic in each method (the seven
methods that currently catch NetworkError and map to DomainError) to call this
helper and rethrow the resulting DomainError.
In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift`:
- Around line 43-44: In NetworkErrorView replace hardcoded .white assignments
(backgroundColor and contentView.backgroundColor) with the project's dynamic
color choice (e.g. .systemBackground or the appropriate BitnagilColor constant)
so the view follows dark mode and project conventions; update the assignments in
the NetworkErrorView setup/initializer where backgroundColor and
contentView.backgroundColor are set to use the chosen color.
In `@Projects/Presentation/Sources/Common/Protocol/BaseViewController.swift`:
- Around line 47-53: The networkErrorView is being added in
BaseViewController.configureLayout before subclass views when subclasses call
super.configureLayout() first, causing it to sit behind subclass content; update
the layout flow so networkErrorView is always topmost by either (A) moving the
networkErrorView addition and its constraints out of configureLayout into a
point after subclass layout (e.g., require subclasses to call
super.configureLayout() last) or (B) keep it in configureLayout but immediately
call bringSubviewToFront(networkErrorView) (or call handleNetworkErrorView) at
the end of configureLayout to ensure networkErrorView is brought above any
subclass-added views; reference networkErrorView, configureLayout,
handleNetworkErrorView, bringSubviewToFront and super.configureLayout() when
making the change.
In `@Projects/Presentation/Sources/Home/View/HomeViewController.swift`:
- Around line 263-265: The dimmedView/floatingMenu/floatingButton are added
after super.configureLayout() which added networkErrorView, causing z-order
conflicts when a network error occurs while the floating menu is open; update
handleNetworkErrorView to first close or hide the floating menu and dimmedView
(e.g., call floatingMenu.dismiss()/close() or set floatingMenu.isHidden = true
and set dimmedView.isHidden = true/removeFromSuperview()) before presenting or
bringing networkErrorView to front, or alternatively ensure you call
view.bringSubviewToFront(networkErrorView) and then hide/remove dimmedView so
the networkErrorView is never obscured.
In `@Projects/Presentation/Sources/Report/ViewModel/ReportHistoryViewModel.swift`:
- Around line 168-174: The catch currently only passes errors to
networkRetryHandler.handleNetworkError (which handles DomainError.requireRetry)
so non-network errors (.business, .unknown) are ignored; update the catch in
fetchReports to distinguish DomainError.requireRetry vs other errors by
inspecting the thrown error, call networkRetryHandler.handleNetworkError for
retryable DomainError cases, and for .business/.unknown paths invoke a
user-feedback mechanism (e.g., showToast/showAlert/logError) and keep the
existing TODO comment removed; reference networkRetryHandler.handleNetworkError,
fetchReports, and DomainError.requireRetry/.business/.unknown when adding the
conditional handling and feedback call.
Comment on lines
+37
to
+52
| do { | ||
| guard let response = try await networkService.request(endpoint: endpoint, type: EmotionResponseDTO.self) | ||
| else { throw NetworkError.unknown(description: "Emotion Reponse를 받아오지 못했습니다.") } | ||
|
|
||
| let emotionEntity = response.toEmotionEntity() | ||
| return emotionEntity | ||
| } catch let error as NetworkError { | ||
| switch error { | ||
| case .needRetry, .invalidURL, .emptyData: | ||
| throw DomainError.requireRetry | ||
| default: | ||
| throw DomainError.business(error.description) | ||
| } | ||
| } catch { | ||
| throw DomainError.unknown | ||
| } |
There was a problem hiding this comment.
loadEmotion에서 NetworkError를 직접 throw한 후 같은 메서드 내에서 다시 catch하는 것은 우회적입니다.
Line 39에서 response가 nil일 때 NetworkError.unknown을 throw하는데, 이는 바로 아래 catch let error as NetworkError에서 잡혀 DomainError.business로 변환됩니다. 직접 DomainError를 throw하는 것이 더 명확합니다. 또한 "Reponse"는 "Response"의 오타입니다.
🛠️ 수정 제안
do {
guard let response = try await networkService.request(endpoint: endpoint, type: EmotionResponseDTO.self)
- else { throw NetworkError.unknown(description: "Emotion Reponse를 받아오지 못했습니다.") }
+ else { throw DomainError.business("Emotion Response를 받아오지 못했습니다.") }
let emotionEntity = response.toEmotionEntity()
return emotionEntity🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/DataSource/Sources/Repository/EmotionRepository.swift` around lines
37 - 52, In loadEmotion, don’t throw a NetworkError when the response is nil
then immediately catch and map it — throw the appropriate DomainError directly
instead; replace the guard’s throw NetworkError.unknown(...) with throw
DomainError.business("Emotion Response를 받아오지 못했습니다.") (fixing the "Reponse" ->
"Response" typo), so the downstream catch for NetworkError in the same method no
longer needs to translate this case.
Comment on lines
+17
to
+30
| do { | ||
| guard let response = try await networkService.request(endpoint: endpoint, type: OnboardingResponseDTO.self) | ||
| else { throw UserError.onboardingLoadFailed } | ||
|
|
||
| let onboardingEntity = response.toOnboardingEntity() | ||
| return onboardingEntity | ||
| } catch let error as NetworkError { | ||
| switch error { | ||
| case .needRetry, .invalidURL, .emptyData: | ||
| throw DomainError.requireRetry | ||
| default: | ||
| throw DomainError.business(error.description) | ||
| } | ||
| } |
There was a problem hiding this comment.
loadOnboardingResult에 일반 catch 블록이 누락되었습니다.
다른 모든 리포지토리 메서드(registerOnboarding, registerRecommendedRoutines 포함)에는 catch { throw DomainError.unknown } 블록이 있지만, 이 메서드에만 누락되었습니다. Line 19에서 throw UserError.onboardingLoadFailed가 do 블록 안에서 발생하면 catch let error as NetworkError에 매칭되지 않아 UserError가 그대로 전파됩니다. 의도적이라면 괜찮지만, 일관성을 위해 확인이 필요합니다.
🐛 일관성을 위한 수정 제안
UserError.onboardingLoadFailed를 의도적으로 전파하려면 do 블록 밖으로 이동하거나, 일반 catch를 추가하세요:
} catch let error as NetworkError {
switch error {
case .needRetry, .invalidURL, .emptyData:
throw DomainError.requireRetry
default:
throw DomainError.business(error.description)
}
+ } catch let error as UserError {
+ throw error
+ } catch {
+ throw DomainError.unknown
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/DataSource/Sources/Repository/OnboardingRepository.swift` around
lines 17 - 30, The method loadOnboardingResult currently only catches
NetworkError (catch let error as NetworkError) so non-network errors like
UserError.onboardingLoadFailed will escape; to align with other repository
methods (e.g., registerOnboarding, registerRecommendedRoutines) add a general
catch after the NetworkError catch that throws DomainError.unknown, or
alternatively move the throw UserError.onboardingLoadFailed outside the do so
it’s handled consistently; update loadOnboardingResult accordingly to ensure all
non-NetworkError exceptions are mapped to DomainError.unknown.
Collaborator
Author
There was a problem hiding this comment.
UserError.onboardingLoadFailed 를 그대로 throw 하기 위한 의도된 부분입니다
There was a problem hiding this comment.
@taipaise 이해했습니다! UserError.onboardingLoadFailed를 DomainError로 변환하지 않고 그대로 전파하기 위한 의도된 설계라는 것을 확인했습니다. 감사합니다.
✏️ Learnings added
Learnt from: taipaise
Repo: YAPP-Github/Bitnagil-iOS PR: 85
File: Projects/DataSource/Sources/Repository/OnboardingRepository.swift:17-30
Timestamp: 2026-02-17T11:00:01.598Z
Learning: OnboardingRepository의 loadOnboardingResult 메서드에서 UserError.onboardingLoadFailed는 의도적으로 DomainError로 변환하지 않고 그대로 throw 됩니다. 이는 다른 메서드들(registerOnboarding, registerRecommendedRoutines)과 달리 일반 catch 블록이 없는 이유이며, NetworkError만 DomainError로 변환하고 UserError는 원본 그대로 전파하는 것이 의도된 동작입니다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: choijungp
Repo: YAPP-Github/Bitnagil-iOS PR: 19
File: Projects/Domain/Sources/UseCase/Onboarding/OnboardingUseCase.swift:0-0
Timestamp: 2025-07-16T10:58:30.858Z
Learning: choijungp는 OnboardingUseCase의 convertToDictionary 함수에서 에러 처리 개선을 추후 고민하고 반영할 계획을 가지고 있음.
Learnt from: choijungp
Repo: YAPP-Github/Bitnagil-iOS PR: 19
File: Projects/Presentation/Sources/Onboarding/View/OnboardingRecommendedRoutineView.swift:57-59
Timestamp: 2025-07-16T09:21:15.038Z
Learning: OnboardingRecommendedRoutineView에서 viewWillAppear에 registerOnboarding 호출하는 것이 적절한 이유: 사용자가 이전 페이지에서 온보딩 선택지를 변경한 후 돌아올 때 새로운 선택지로 다시 등록해야 하기 때문. 홈 뷰에서는 이 뷰로 돌아올 수 없어서 중복 호출 문제가 발생하지 않음.
Comment on lines
+29
to
+38
| } catch let error as NetworkError { | ||
| switch error { | ||
| case .needRetry, .invalidURL, .emptyData: | ||
| throw DomainError.requireRetry | ||
| default: | ||
| throw DomainError.business(error.description) | ||
| } | ||
| } catch { | ||
| throw DomainError.unknown | ||
| } |
There was a problem hiding this comment.
.invalidURL을 DomainError.requireRetry로 매핑하는 것은 부적절합니다.
invalidURL은 프로그래밍 오류로 재시도해도 해결되지 않습니다. emptyData도 마찬가지로 재시도로 해결될 가능성이 낮습니다. 사용자에게 "다시 시도" 화면을 보여줘야 하는 케이스는 .needRetry(네트워크 연결 문제)만 해당될 것으로 보입니다.
🛠️ 에러 매핑 수정 제안
} catch let error as NetworkError {
switch error {
- case .needRetry, .invalidURL, .emptyData:
+ case .needRetry:
throw DomainError.requireRetry
default:
throw DomainError.business(error.description)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch let error as NetworkError { | |
| switch error { | |
| case .needRetry, .invalidURL, .emptyData: | |
| throw DomainError.requireRetry | |
| default: | |
| throw DomainError.business(error.description) | |
| } | |
| } catch { | |
| throw DomainError.unknown | |
| } | |
| } catch let error as NetworkError { | |
| switch error { | |
| case .needRetry: | |
| throw DomainError.requireRetry | |
| default: | |
| throw DomainError.business(error.description) | |
| } | |
| } catch { | |
| throw DomainError.unknown | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/DataSource/Sources/Repository/RoutineRepository.swift` around lines
29 - 38, The catch block in RoutineRepository currently maps
NetworkError.invalidURL and .emptyData to DomainError.requireRetry; change it so
only .needRetry maps to DomainError.requireRetry, while .invalidURL and
.emptyData are mapped to non-retry errors (e.g.,
DomainError.business(error.description) or DomainError.unknown as appropriate).
Update the catch handling for "catch let error as NetworkError" to switch
.needRetry -> DomainError.requireRetry and handle .invalidURL and .emptyData by
throwing a business/unknown domain error, leaving other cases to the default
branch that throws DomainError.business(error.description).
Collaborator
Author
There was a problem hiding this comment.
인지하고 있으며, 추후 수정하기 위해 TODO List에 넣어놨습니다
There was a problem hiding this comment.
@taipaise 네, 이해했습니다! 나중에 수정하실 계획이시군요.
필요하시다면 이 TODO 항목을 추적하기 위해 GitHub 이슈를 생성해드릴 수 있습니다. 이슈에는 이 PR과 코멘트 링크를 포함하여 나중에 쉽게 찾아볼 수 있도록 하겠습니다. 원하시면 말씀해주세요!
🧠 Learnings used
Learnt from: taipaise
Repo: YAPP-Github/Bitnagil-iOS PR: 85
File: Projects/DataSource/Sources/Repository/OnboardingRepository.swift:17-30
Timestamp: 2026-02-17T11:00:01.598Z
Learning: OnboardingRepository의 loadOnboardingResult 메서드에서 UserError.onboardingLoadFailed는 의도적으로 DomainError로 변환하지 않고 그대로 throw 됩니다. 이는 다른 메서드들(registerOnboarding, registerRecommendedRoutines)과 달리 일반 catch 블록이 없는 이유이며, NetworkError만 DomainError로 변환하고 UserError는 원본 그대로 전파하는 것이 의도된 동작입니다.
Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift
Outdated
Show resolved
Hide resolved
Comment on lines
+60
to
+61
| retryButton.tintColor = BitnagilColor.gray10 | ||
| retryButton.backgroundColor = BitnagilColor.gray10 |
There was a problem hiding this comment.
tintColor와 backgroundColor가 동일한 값(BitnagilColor.gray10)으로 설정되어 있습니다.
tintColor는 버튼의 이미지나 시스템 아이콘에 영향을 주는데, backgroundColor와 동일하게 설정하면 향후 아이콘 추가 시 보이지 않을 수 있습니다. tintColor 설정이 의도적인지 확인해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift` around
lines 60 - 61, retryButton currently sets both retryButton.tintColor and
retryButton.backgroundColor to BitnagilColor.gray10 which will hide any icon or
system image; inspect the NetworkErrorView (or the retryButton setup) and either
remove the tintColor assignment if you want the default system tint, or change
retryButton.tintColor to a contrasting color (e.g., BitnagilColor.white or a
semantic accent color) so icons remain visible while leaving
retryButton.backgroundColor as BitnagilColor.gray10.
...sentation/Sources/ResultRecommendedRoutine/ViewModel/ResultRecommendedRoutineViewModel.swift
Show resolved
Hide resolved
Projects/Presentation/Sources/RoutineList/ViewModel/RoutineListViewModel.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Projects/Presentation/Sources/Common/Protocol/BaseViewController.swift (1)
79-91: 탭 바를 직접 숨기는 VC에서tabBar.isHidden = false복원이 충돌할 수 있습니다.
handleNetworkErrorView(show: false)및deinit에서tabBar.isHidden = false를 설정하는데,hidesBottomBarWhenPushed = true로 push된 VC에서 네트워크 에러가 발생 후 복구되면 탭 바가 의도치 않게 표시될 수 있습니다.현재 사용처가 탭 레벨 화면에 한정되어 있다면 문제가 없지만, 향후 확장 시 주의가 필요합니다. 네트워크 에러 표시 전 탭 바의 이전 상태를 저장하고 복원하는 방식을 고려해 보세요.
♻️ 수정 제안
+ private var previousTabBarHiddenState: Bool = false + private func handleNetworkErrorView(show: Bool) { isShowingNetworkError = show if show { + previousTabBarHiddenState = tabBarController?.tabBar.isHidden ?? false view.bringSubviewToFront(networkErrorView) networkErrorView.isHidden = false tabBarController?.tabBar.isHidden = true } else { networkErrorView.isHidden = true - tabBarController?.tabBar.isHidden = false + tabBarController?.tabBar.isHidden = previousTabBarHiddenState } }
deinit도 동일하게 적용:deinit { guard isShowingNetworkError else { return } DispatchQueue.main.async { [weak tabBarController = self.tabBarController] in - tabBarController?.tabBar.isHidden = false + tabBarController?.tabBar.isHidden = self.previousTabBarHiddenState } }주의:
deinit에서self는 이미 해제 중이므로,previousTabBarHiddenState값도 캡처 리스트에 포함해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Common/Protocol/BaseViewController.swift` around lines 79 - 91, handleNetworkErrorView currently forces tabBarController?.tabBar.isHidden = true/false which can incorrectly show the tab bar for VCs pushed with hidesBottomBarWhenPushed; save the tab bar's previous hidden state before hiding (e.g. store previousTabBarHiddenState when show==true), restore that saved state when show==false, and also restore it in deinit (capture the saved previousTabBarHiddenState in the deinit closure/context since self may be deallocating). Update references: handleNetworkErrorView, isShowingNetworkError, networkErrorView, tabBarController?.tabBar.isHidden and deinit to use this save-and-restore approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift`:
- Around line 60-61: The retryButton currently sets both tintColor and
backgroundColor to BitnagilColor.gray10 which will hide any templated image or
system icon; update the retryButton configuration (the retryButton instance) to
use a contrasting tintColor (e.g., .white) or remove the tintColor assignment
entirely so icons remain visible while keeping backgroundColor as
BitnagilColor.gray10.
---
Nitpick comments:
In `@Projects/Presentation/Sources/Common/Protocol/BaseViewController.swift`:
- Around line 79-91: handleNetworkErrorView currently forces
tabBarController?.tabBar.isHidden = true/false which can incorrectly show the
tab bar for VCs pushed with hidesBottomBarWhenPushed; save the tab bar's
previous hidden state before hiding (e.g. store previousTabBarHiddenState when
show==true), restore that saved state when show==false, and also restore it in
deinit (capture the saved previousTabBarHiddenState in the deinit
closure/context since self may be deallocating). Update references:
handleNetworkErrorView, isShowingNetworkError, networkErrorView,
tabBarController?.tabBar.isHidden and deinit to use this save-and-restore
approach.
taipaise
force-pushed
the
feat/network-error
branch
from
18d3473 to
7df5255
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 (1)
Projects/Presentation/Sources/Report/View/ReportCompleteViewController.swift (1)
83-88:⚠️ Potential issue | 🟠 Major
configureAttribute()와configureLayout()이 두 번 호출됩니다.
BaseViewController.viewDidLoad()에서 이미configureAttribute(),configureLayout(),bind()를 호출하고 있으므로, Line 85-86에서 다시 명시적으로 호출하면 이 메서드들이 두 번 실행됩니다.이번 PR에서
super.configureAttribute()와super.configureLayout()호출이 추가되었기 때문에,BaseViewController.configureLayout()이 두 번 실행되면서networkErrorView에 SnapKit 제약 조건이 중복 추가될 수 있습니다.Line 85-86의 명시적 호출을 제거하고,
viewModel.action호출만 남기는 것을 권장합니다.제안된 수정
override func viewDidLoad() { super.viewDidLoad() - configureAttribute() - configureLayout() viewModel.action(input: .fetchReportDetail(reportId: reportId)) }Based on learnings: "BaseViewController의 viewDidLoad() 메서드에서 이미 configureAttribute(), configureLayout(), bind()를 호출하므로, 하위 클래스에서 super.viewDidLoad()를 호출하면 이 메서드들이 자동으로 호출된다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Report/View/ReportCompleteViewController.swift` around lines 83 - 88, The subclass ReportCompleteViewController currently calls configureAttribute() and configureLayout() explicitly in viewDidLoad() even though BaseViewController.viewDidLoad() already invokes super.configureAttribute(), super.configureLayout(), and bind(); remove the duplicate calls on Lines 85–86 so viewDidLoad() only calls super.viewDidLoad() and then the remaining unique work (keep the existing viewModel.action(input: .fetchReportDetail(reportId: reportId)) call) to avoid double-running configureAttribute()/configureLayout() and duplicate SnapKit constraints on networkErrorView.
🧹 Nitpick comments (9)
Projects/Presentation/Sources/RoutineList/View/RoutineDeleteAlertViewController.swift (1)
56-57:super호출 패턴은 PR 전체와 일관성 있음 — 다만 이 VC에서의 실효성 검토 필요.이 VC는 루틴 삭제 확인용 소형 모달(alert)인데,
super.configureLayout()이networkErrorView를view에 full-screen으로 추가합니다.bindNetworkError를 호출하지 않으므로 실제로 보이지는 않지만, 불필요한 뷰가 뷰 계층에 추가됩니다.현재 동작에 문제가 있는 것은 아니며, PR 전반의 일관된 패턴을 따른 것으로 이해됩니다. 다만 모달/alert 스타일 VC에는 네트워크 에러 뷰가 필요 없으므로, 향후
BaseViewController를 네트워크 에러가 필요한 VC와 그렇지 않은 VC로 분리하거나, opt-in 방식으로 전환하는 것을 고려해 볼 수 있습니다.Also applies to: 108-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/RoutineList/View/RoutineDeleteAlertViewController.swift` around lines 56 - 57, The controller currently calls super.configureLayout() which adds the full-screen networkErrorView into the view hierarchy even though this alert VC doesn't use network errors; in RoutineDeleteAlertViewController remove that unnecessary view by either not calling super.configureLayout() (keep super.configureAttribute() if needed) or call super.configureLayout() then remove networkErrorView from self.view, or introduce an opt-in flag on BaseViewController so configureLayout only adds networkErrorView when enabled; reference configureAttribute, configureLayout, networkErrorView, bindNetworkError, and BaseViewController when making the change.Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift (1)
24-29:onRetry의 접근 제어 수준을 명시하는 것을 고려해 주세요.
onRetry가 명시적 접근 제어자 없이 선언되어internal로 동작합니다. 의도에 맞다면 괜찮지만, 나머지 프로퍼티들이private으로 선언된 것과의 일관성을 위해 명시적으로 표기하면 가독성이 향상됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift` around lines 24 - 29, Explicitly annotate the access level for the onRetry property in NetworkErrorView to remove the implicit internal and improve consistency with the other private properties; update the declaration of onRetry to the intended access level (for example: internal var onRetry: (() -> Void)? if it should remain module-visible, or public var onRetry: (() -> Void)? if it must be exposed to clients, or private(set) var onRetry: (() -> Void)? if external read-only access is desired).Projects/DataSource/Sources/Repository/RecommendedRoutineRepository.swift (1)
47-56: 에러 매핑 로직이 두 메서드에서 중복됨
NetworkError→DomainError변환 로직이fetchRecommendedRoutine과fetchRecommendedRoutines에 동일하게 반복되며, AI 요약에 따르면 다른 Repository 파일들에서도 같은 패턴이 사용됩니다. 헬퍼 함수나Errorextension으로 추출하면 일관성 유지와 향후 매핑 변경 시 수정 포인트를 줄일 수 있습니다.예시: NetworkError extension 추출
// 예: NetworkError+DomainError.swift extension NetworkError { func toDomainError() -> DomainError { switch self { case .needRetry: return .requireRetry default: return .business(self.description) } } }그러면 각 catch 블록이 아래처럼 간결해집니다:
- } catch let error as NetworkError { - switch error { - case .needRetry, .invalidURL, .emptyData: - throw DomainError.requireRetry - default: - throw DomainError.business(error.description) - } + } catch let error as NetworkError { + throw error.toDomainError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/DataSource/Sources/Repository/RecommendedRoutineRepository.swift` around lines 47 - 56, The NetworkError→DomainError mapping is duplicated in fetchRecommendedRoutine and fetchRecommendedRoutines; extract that logic into a single helper (e.g., add NetworkError extension with func toDomainError() -> DomainError or a static mapper) and replace the catch let error as NetworkError blocks in RecommendedRoutineRepository.fetchRecommendedRoutine and fetchRecommendedRoutines to throw error.toDomainError() (or the helper result); update other repositories using the same pattern to call the same helper so mapping stays consistent.Projects/DataSource/Sources/Repository/OnboardingRepository.swift (1)
41-56: 중복된 에러 매핑 로직을 헬퍼로 추출하는 것을 권장
NetworkError→DomainError변환 switch 문이 세 메서드에 동일하게 반복되고 있습니다. 다른 Repository 파일들에서도 같은 패턴이 사용될 것으로 보이므로, 공통 헬퍼 함수나NetworkErrorextension으로 추출하면 유지보수성이 향상됩니다.♻️ 리팩터링 예시
NetworkErrorextension 또는 별도 매퍼 함수:extension NetworkError { func toDomainError() -> DomainError { switch self { case .needRetry, .emptyData: return .requireRetry default: return .business(self.description) } } }사용 시:
} catch let error as NetworkError { - switch error { - case .needRetry, .invalidURL, .emptyData: - throw DomainError.requireRetry - default: - throw DomainError.business(error.description) - } + throw error.toDomainError() } catch {Also applies to: 62-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/DataSource/Sources/Repository/OnboardingRepository.swift` around lines 41 - 56, The NetworkError-to-DomainError switch repeated in OnboardingRepository's async request handlers should be extracted into a single helper (e.g., an extension method like NetworkError.toDomainError() or a shared mapper function); replace the switch in the catch let error as NetworkError blocks with throw error.toDomainError() (or throw map(error)) and ensure the mapping covers .needRetry and .emptyData → .requireRetry and all other cases → .business(error.description), leaving the existing generic catch to throw .unknown.Projects/DataSource/Sources/Repository/RoutineRepository.swift (1)
27-38: 반복되는 에러 변환 로직을 헬퍼로 추출하는 것을 권장합니다.7개 메서드에 동일한 do-catch + NetworkError→DomainError 변환 블록이 복사되어 있습니다. 헬퍼 함수로 추출하면 유지보수성이 크게 향상됩니다.
♻️ 리팩토링 제안
+ private func performNetworkRequest<T: Decodable>( + endpoint: Endpoint, + type: T.Type + ) async throws -> T? { + do { + return try await networkService.request(endpoint: endpoint, type: type) + } catch let error as NetworkError { + switch error { + case .needRetry: + throw DomainError.requireRetry + default: + throw DomainError.business(error.description) + } + } catch { + throw DomainError.unknown + } + }각 메서드에서 사용 예시:
func createRoutine(routine: RoutineCreationEntity) async throws { // ... DTO 생성 ... let endpoint = RoutineEndpoint.createRoutine(routine: routineCreationDTO) _ = try await performNetworkRequest(endpoint: endpoint, type: EmptyResponseDTO.self) }Also applies to: 44-57, 63-83, 99-110, 116-127, 133-144, 156-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/DataSource/Sources/Repository/RoutineRepository.swift` around lines 27 - 38, Several methods in RoutineRepository duplicate the same do-catch that calls networkService.request and maps NetworkError cases (.needRetry, .invalidURL, .emptyData) to DomainError.requireRetry and other errors to DomainError.business(error.description); extract that logic into a helper (e.g., performNetworkRequest(endpoint:type:)) and replace each duplicate block (seen in methods like createRoutine which currently calls networkService.request with EmptyResponseDTO.self) with a single call to the helper; ensure the helper performs the await networkService.request(endpoint:type:), catches NetworkError and maps the same cases to DomainError.requireRetry and others to DomainError.business(error.description), and rethrows DomainError.Projects/Presentation/Sources/Common/Protocol/NetworkRetryCapable.swift (1)
12-14:CurrentValueSubject에 클로저를 저장하면 retain cycle 위험이 있습니다.
networkErrorActionSubject가(() -> Void)?타입의 클로저를 보유하고, 호출부에서[weak self]로 캡처하지 않으면 ViewModel이 해제되지 않을 수 있습니다. 프로토콜 doc comment에[weak self]사용을 명시하거나,clearRetryState()가 적시에 호출되는지 확인이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Common/Protocol/NetworkRetryCapable.swift` around lines 12 - 14, The protocol stores a closure in networkErrorActionSubject which can create retain cycles; update the doc comment on NetworkRetryCapable.networkErrorActionSubject to explicitly require callers to capture [weak self] (or otherwise avoid strong self) when assigning closures and to state that implementers must call clearRetryState() (or nil out the subject) promptly (e.g., in deinit or after a retry); alternatively note that implementers may replace the stored closure with a weak-wrapper type if they prefer to avoid relying on caller discipline.Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift (3)
126-156:loadNickname과fetchEmotion에서 동시 실패 시 하나의 retry action만 유지됩니다.
HomeViewModel은 초기 로딩 시loadNickname,fetchEmotion,fetchRoutines,checkVersion등을 동시에 호출할 가능성이 높습니다. 네트워크가 끊긴 상태에서는 모든 호출이 실패하지만,CurrentValueSubject는 마지막 retry action만 보관합니다.사용자가 "다시 시도"를 누르면 하나의 호출만 재시도되고, 그 성공 시
clearRetryState()가 에러 뷰를 숨기므로 나머지 실패한 호출들은 재시도되지 않습니다.권장안:
HomeViewModel의 경우, retry action에서 모든 초기 로딩 호출을 다시 수행하는 단일 클로저를 사용하는 것이 더 안정적입니다:💡 예시
// 초기 로딩 실패 시 모든 호출을 재시도하는 방식 networkRetryHandler.handleNetworkError(error) { [weak self] in self?.loadNickname() self?.fetchEmotion() self?.fetchRoutines() self?.checkVersion() }또는 초기 로딩을 하나의 메서드로 묶어서 재시도 대상으로 설정할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift` around lines 126 - 156, The retry handler currently stores only the last retry closure, so when multiple initial load methods (loadNickname, fetchEmotion, fetchRoutines, checkVersion) fail concurrently only one will be retried; change each catch block to register a single retry action that re-runs all initial load steps (or call a new unified method like performInitialLoad that invokes loadNickname(), fetchEmotion(), fetchRoutines(), checkVersion()), and keep using networkRetryHandler.handleNetworkError(error) with that combined closure and preserve calling networkRetryHandler.clearRetryState() on success.
221-229:fetchRoutines(startDate:endDate:)재시도 시 파라미터 캡처가 올바릅니다.
startDate와endDate가 클로저에 정확히 캡처되어, 재시도 시 원래 요청과 동일한 범위를 조회합니다.Line 225의
TODO: - 네트워크 에러 제외 오류 처리가 남아 있으니, 향후 비네트워크 에러에 대한 처리도 추가하시면 좋겠습니다.비네트워크 에러 처리 로직 구현을 도와드릴까요? 이슈를 생성해 드릴 수도 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift` around lines 221 - 229, The TODO indicates missing handling for non-network errors in fetchRoutines(startDate:endDate:); implement logic to distinguish network errors from others inside the catch where networkRetryHandler.handleNetworkError(...) is called: use the caught error to check/branch (e.g., isNetworkError predicate or error type), for non-network errors send a meaningful failure state via fetchRoutineResultSubject, log or report the error (use existing logger), and surface/localize a user-facing error message or error state so the UI can react; update the catch block around fetchRoutines to perform this non-network error branch while keeping the existing networkRetryHandler.handleNetworkError(...) call for network errors and retaining captured startDate/endDate for retries.
301-324:checkVersion실패 시 네트워크 재시도가 적용되었습니다.버전 체크는 앱 시작 시 한 번 호출되는 기능으로, 네트워크 에러 시 재시도 대상으로 포함시킨 것은 합리적입니다. 다만 버전 체크 실패가 네트워크 에러 뷰를 표시하는 것이 사용자 경험상 적절한지 검토해 주세요 — 버전 체크 실패는 사일런트 처리하는 것이 더 적합할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift` around lines 301 - 324, The current checkVersion() uses networkRetryHandler.handleNetworkError(...) on any failure which triggers the network error UI; instead, treat version check failures silently: in the catch block for checkVersion(), do not call networkRetryHandler.handleNetworkError — simply log the error (or ignore), send updateVersionSubject.send(nil) to clear any prompt, and optionally call networkRetryHandler.clearRetryState(); update references in the method (checkVersion, appConfigRepository.fetchAppVersion, updateVersionSubject, networkRetryHandler) accordingly so the version check failure does not surface the network error view to users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Projects/DataSource/Sources/Repository/LocationRepository.swift`:
- Around line 53-59: The catch mapping in LocationRepository currently treats
.invalidURL and .emptyData as transient and throws DomainError.requireRetry;
change the switch in the NetworkError catch so only .needRetry maps to
DomainError.requireRetry, and map .invalidURL and .emptyData to
DomainError.business with the error description (i.e., treat them as
non-retryable business errors). Update the same pattern in the other
repositories that share this logic (EmotionRepository, FileRepository,
OnboardingRepository, RecommendedRoutineRepository, ReportRepository,
RoutineRepository) so all use .needRetry => DomainError.requireRetry and all
other NetworkError cases => DomainError.business(error.description).
In `@Projects/DataSource/Sources/Repository/OnboardingRepository.swift`:
- Around line 24-29: The switch currently maps `.invalidURL` to
DomainError.requireRetry along with `.needRetry` and `.emptyData`; change the
mapping so only transient errors (`.needRetry` and `.emptyData`) throw
DomainError.requireRetry, while `.invalidURL` should throw
DomainError.business(error.description). Update this logic in the switch blocks
inside OnboardingRepository's three methods (the switch handling `error` that
currently lists `.needRetry, .invalidURL, .emptyData`) and apply the same fix
consistently across other repositories that repeat this pattern (e.g.,
RoutineRepository, FileRepository, ReportRepository) so `.invalidURL` is treated
as a business error while retryable cases remain requireRetry.
In `@Projects/DataSource/Sources/Repository/RecommendedRoutineRepository.swift`:
- Around line 22-27: The switch incorrectly maps .invalidURL and .emptyData to
DomainError.requireRetry (only .needRetry should trigger retries); update the
switch in RecommendedRoutineRepository (and the same pattern in
LocationRepository, RoutineRepository, ReportRepository, OnboardingRepository,
FileRepository, EmotionRepository, etc.) so that case .needRetry throws
DomainError.requireRetry, while .invalidURL and .emptyData (and other
non-transient cases) throw DomainError.business(error.description) (preserving
the existing default behavior) to avoid prompting retries for permanent errors.
In `@Projects/Presentation/Sources/Common/Protocol/NetworkRetryCapable.swift`:
- Around line 27-37: handleNetworkError currently returns silently for
non-DomainError.requireRetry errors, causing DomainError.business/unknown to be
ignored; update handleNetworkError to log or forward non-retry errors instead of
silently returning: inside the else branch (where you now comment "// 네트워크 에러
문제가 아닐 경우") call a logger (or send the error to an existing error reporting
subject) and/or expose outcome to callers (e.g., return a Bool handled flag or
add a nonRetryErrorSubject) so ViewModels can surface or handle
DomainError.business/unknown; refer to handleNetworkError,
DomainError.requireRetry, and networkErrorActionSubject when making the change.
In
`@Projects/Presentation/Sources/ResultRecommendedRoutine/ViewModel/ResultRecommendedRoutineViewModel.swift`:
- Around line 166-173: In ResultRecommendedRoutineViewModel, avoid emitting
registerRoutineResultSubject.send(false) for retryable network errors
(DomainError.requireRetry) so the VC doesn't get a duplicate error UI; modify
the catch in registerRecommendedRoutine() to detect DomainError.requireRetry (or
use networkRetryHandler.isRetryable(error)) and only call
registerRoutineResultSubject.send(false) when the error is not retryable,
leaving networkRetryHandler.handleNetworkError(error) {
self?.registerRecommendedRoutine() } to handle retryable cases; reference
registerRoutineResultSubject.send(false),
networkRetryHandler.handleNetworkError(error){...}, DomainError.requireRetry,
and registerRoutineResultPublisher so VC receives a single error signal.
---
Outside diff comments:
In
`@Projects/Presentation/Sources/Report/View/ReportCompleteViewController.swift`:
- Around line 83-88: The subclass ReportCompleteViewController currently calls
configureAttribute() and configureLayout() explicitly in viewDidLoad() even
though BaseViewController.viewDidLoad() already invokes
super.configureAttribute(), super.configureLayout(), and bind(); remove the
duplicate calls on Lines 85–86 so viewDidLoad() only calls super.viewDidLoad()
and then the remaining unique work (keep the existing viewModel.action(input:
.fetchReportDetail(reportId: reportId)) call) to avoid double-running
configureAttribute()/configureLayout() and duplicate SnapKit constraints on
networkErrorView.
---
Duplicate comments:
In `@Projects/DataSource/Sources/Repository/ReportRepository.swift`:
- Around line 37-50: Update the catch mappings in ReportRepository so only
NetworkError.needRetry maps to DomainError.requireRetry: in the catch blocks
that handle "catch let error as NetworkError" (the one shown plus the catch
blocks in fetchReports and fetchReportDetail), remove .invalidURL and .emptyData
from the .needRetry case and instead handle them as general business errors
(e.g., throw DomainError.business(error.description) or the existing default
branch). Locate the catch around the networkService.request calls in the methods
(the shown request and the ones in fetchReports and fetchReportDetail) and
change the switch so case .needRetry -> throw DomainError.requireRetry, and all
other NetworkError cases fall through to the default business error handling.
In `@Projects/DataSource/Sources/Repository/RoutineRepository.swift`:
- Around line 27-38: The switch in the NetworkError catch is incorrectly mapping
.invalidURL and .emptyData to DomainError.requireRetry; change the mapping so
only .needRetry maps to DomainError.requireRetry and all other NetworkError
cases (including .invalidURL and .emptyData) map to
DomainError.business(error.description) (preserving the default catch to
DomainError.unknown), and apply this same fix to the other six methods that
repeat the pattern (look for networkService.request(...) and the NetworkError
catch block).
In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift`:
- Around line 60-61: The retryButton currently sets both tintColor and
backgroundColor to BitnagilColor.gray10 in NetworkErrorView (retryButton),
causing future icon images to be invisible against the same-colored tint; remove
the unnecessary tintColor assignment (retryButton.tintColor =
BitnagilColor.gray10) so the button will inherit a default/contrasting tint or
explicitly set tintColor to a contrasting color when adding an icon.
---
Nitpick comments:
In `@Projects/DataSource/Sources/Repository/OnboardingRepository.swift`:
- Around line 41-56: The NetworkError-to-DomainError switch repeated in
OnboardingRepository's async request handlers should be extracted into a single
helper (e.g., an extension method like NetworkError.toDomainError() or a shared
mapper function); replace the switch in the catch let error as NetworkError
blocks with throw error.toDomainError() (or throw map(error)) and ensure the
mapping covers .needRetry and .emptyData → .requireRetry and all other cases →
.business(error.description), leaving the existing generic catch to throw
.unknown.
In `@Projects/DataSource/Sources/Repository/RecommendedRoutineRepository.swift`:
- Around line 47-56: The NetworkError→DomainError mapping is duplicated in
fetchRecommendedRoutine and fetchRecommendedRoutines; extract that logic into a
single helper (e.g., add NetworkError extension with func toDomainError() ->
DomainError or a static mapper) and replace the catch let error as NetworkError
blocks in RecommendedRoutineRepository.fetchRecommendedRoutine and
fetchRecommendedRoutines to throw error.toDomainError() (or the helper result);
update other repositories using the same pattern to call the same helper so
mapping stays consistent.
In `@Projects/DataSource/Sources/Repository/RoutineRepository.swift`:
- Around line 27-38: Several methods in RoutineRepository duplicate the same
do-catch that calls networkService.request and maps NetworkError cases
(.needRetry, .invalidURL, .emptyData) to DomainError.requireRetry and other
errors to DomainError.business(error.description); extract that logic into a
helper (e.g., performNetworkRequest(endpoint:type:)) and replace each duplicate
block (seen in methods like createRoutine which currently calls
networkService.request with EmptyResponseDTO.self) with a single call to the
helper; ensure the helper performs the await
networkService.request(endpoint:type:), catches NetworkError and maps the same
cases to DomainError.requireRetry and others to
DomainError.business(error.description), and rethrows DomainError.
In `@Projects/Presentation/Sources/Common/Component/NetworkErrorView.swift`:
- Around line 24-29: Explicitly annotate the access level for the onRetry
property in NetworkErrorView to remove the implicit internal and improve
consistency with the other private properties; update the declaration of onRetry
to the intended access level (for example: internal var onRetry: (() -> Void)?
if it should remain module-visible, or public var onRetry: (() -> Void)? if it
must be exposed to clients, or private(set) var onRetry: (() -> Void)? if
external read-only access is desired).
In `@Projects/Presentation/Sources/Common/Protocol/NetworkRetryCapable.swift`:
- Around line 12-14: The protocol stores a closure in networkErrorActionSubject
which can create retain cycles; update the doc comment on
NetworkRetryCapable.networkErrorActionSubject to explicitly require callers to
capture [weak self] (or otherwise avoid strong self) when assigning closures and
to state that implementers must call clearRetryState() (or nil out the subject)
promptly (e.g., in deinit or after a retry); alternatively note that
implementers may replace the stored closure with a weak-wrapper type if they
prefer to avoid relying on caller discipline.
In `@Projects/Presentation/Sources/Home/ViewModel/HomeViewModel.swift`:
- Around line 126-156: The retry handler currently stores only the last retry
closure, so when multiple initial load methods (loadNickname, fetchEmotion,
fetchRoutines, checkVersion) fail concurrently only one will be retried; change
each catch block to register a single retry action that re-runs all initial load
steps (or call a new unified method like performInitialLoad that invokes
loadNickname(), fetchEmotion(), fetchRoutines(), checkVersion()), and keep using
networkRetryHandler.handleNetworkError(error) with that combined closure and
preserve calling networkRetryHandler.clearRetryState() on success.
- Around line 221-229: The TODO indicates missing handling for non-network
errors in fetchRoutines(startDate:endDate:); implement logic to distinguish
network errors from others inside the catch where
networkRetryHandler.handleNetworkError(...) is called: use the caught error to
check/branch (e.g., isNetworkError predicate or error type), for non-network
errors send a meaningful failure state via fetchRoutineResultSubject, log or
report the error (use existing logger), and surface/localize a user-facing error
message or error state so the UI can react; update the catch block around
fetchRoutines to perform this non-network error branch while keeping the
existing networkRetryHandler.handleNetworkError(...) call for network errors and
retaining captured startDate/endDate for retries.
- Around line 301-324: The current checkVersion() uses
networkRetryHandler.handleNetworkError(...) on any failure which triggers the
network error UI; instead, treat version check failures silently: in the catch
block for checkVersion(), do not call networkRetryHandler.handleNetworkError —
simply log the error (or ignore), send updateVersionSubject.send(nil) to clear
any prompt, and optionally call networkRetryHandler.clearRetryState(); update
references in the method (checkVersion, appConfigRepository.fetchAppVersion,
updateVersionSubject, networkRetryHandler) accordingly so the version check
failure does not surface the network error view to users.
In
`@Projects/Presentation/Sources/RoutineList/View/RoutineDeleteAlertViewController.swift`:
- Around line 56-57: The controller currently calls super.configureLayout()
which adds the full-screen networkErrorView into the view hierarchy even though
this alert VC doesn't use network errors; in RoutineDeleteAlertViewController
remove that unnecessary view by either not calling super.configureLayout() (keep
super.configureAttribute() if needed) or call super.configureLayout() then
remove networkErrorView from self.view, or introduce an opt-in flag on
BaseViewController so configureLayout only adds networkErrorView when enabled;
reference configureAttribute, configureLayout, networkErrorView,
bindNetworkError, and BaseViewController when making the change.
Comment on lines
+53
to
+59
| } catch let error as NetworkError { | ||
| switch error { | ||
| case .needRetry, .invalidURL, .emptyData: | ||
| throw DomainError.requireRetry | ||
| default: | ||
| throw DomainError.business(error.description) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# LocationRepository.swift 파일의 실제 내용 확인 (53-59줄 포함)
find . -name "LocationRepository.swift" -type fRepository: YAPP-Github/Bitnagil-iOS
Length of output: 133
🏁 Script executed:
# NetworkError 정의 확인
find . -name "*.swift" -type f | xargs grep -l "enum NetworkError" | head -5Repository: YAPP-Github/Bitnagil-iOS
Length of output: 131
🏁 Script executed:
# 다른 Repository 파일들에서 동일한 패턴 검색
find . -path "*/Repository/*" -name "*Repository.swift" -type f | head -10Repository: YAPP-Github/Bitnagil-iOS
Length of output: 728
🏁 Script executed:
# LocationRepository.swift 파일의 lines 47-62 확인
cat -n ./Projects/DataSource/Sources/Repository/LocationRepository.swift | sed -n '40,65p'Repository: YAPP-Github/Bitnagil-iOS
Length of output: 1238
🏁 Script executed:
# NetworkError 정의 확인
cat -n ./Projects/DataSource/Sources/NetworkService/NetworkError.swiftRepository: YAPP-Github/Bitnagil-iOS
Length of output: 1311
🏁 Script executed:
# 다른 Repository들의 에러 처리 패턴 확인
for file in ./Projects/DataSource/Sources/Repository/{AppConfigRepository,AuthRepository,EmotionRepository,FileRepository,OnboardingRepository,RecommendedRoutineRepository,ReportRepository,RoutineRepository,UserDataRepository}.swift; do
echo "=== $file ===";
grep -A 10 "catch let error as NetworkError" "$file" | head -15;
doneRepository: YAPP-Github/Bitnagil-iOS
Length of output: 3602
.invalidURL와 .emptyData를 requireRetry로 매핑하는 것이 부적절함
.needRetry는 타임아웃/연결 끊김 등 일시적 네트워크 오류이므로 재시도가 의미 있습니다. 그러나 .invalidURL은 URL 구성 오류로 재시도해도 동일하게 실패하며, .emptyData는 서버 응답 자체가 비어있는 경우라 클라이언트 재시도로 해결될 가능성이 낮습니다.
이 문제는 LocationRepository에만 국한된 것이 아니라 EmotionRepository, FileRepository, OnboardingRepository, RecommendedRoutineRepository, ReportRepository, RoutineRepository 등 다수의 Repository에서 동일한 패턴으로 반복되고 있습니다. 이는 체계적인 설계 문제로, 다음과 같이 수정하는 것이 적절합니다:
제안하는 수정
} catch let error as NetworkError {
switch error {
- case .needRetry, .invalidURL, .emptyData:
+ case .needRetry:
throw DomainError.requireRetry
+ case .invalidURL, .emptyData:
+ throw DomainError.business(error.description)
default:
throw DomainError.business(error.description)
}이 수정은 LocationRepository뿐만 아니라 동일한 패턴을 사용하는 모든 Repository 파일에 적용되어야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/DataSource/Sources/Repository/LocationRepository.swift` around lines
53 - 59, The catch mapping in LocationRepository currently treats .invalidURL
and .emptyData as transient and throws DomainError.requireRetry; change the
switch in the NetworkError catch so only .needRetry maps to
DomainError.requireRetry, and map .invalidURL and .emptyData to
DomainError.business with the error description (i.e., treat them as
non-retryable business errors). Update the same pattern in the other
repositories that share this logic (EmotionRepository, FileRepository,
OnboardingRepository, RecommendedRoutineRepository, ReportRepository,
RoutineRepository) so all use .needRetry => DomainError.requireRetry and all
other NetworkError cases => DomainError.business(error.description).
Comment on lines
+24
to
+29
| switch error { | ||
| case .needRetry, .invalidURL, .emptyData: | ||
| throw DomainError.requireRetry | ||
| default: | ||
| throw DomainError.business(error.description) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the NetworkError enum definition
find . -name "*.swift" -type f | xargs grep -l "enum NetworkError"Repository: YAPP-Github/Bitnagil-iOS
Length of output: 131
🏁 Script executed:
# Look at the OnboardingRepository.swift file to see all error handling
cat -n Projects/DataSource/Sources/Repository/OnboardingRepository.swiftRepository: YAPP-Github/Bitnagil-iOS
Length of output: 3369
🏁 Script executed:
# Search for all occurrences of invalidURL case in the codebase
rg "case.*invalidURL" --type swift -B 2 -A 2Repository: YAPP-Github/Bitnagil-iOS
Length of output: 11831
🏁 Script executed:
# Find where NetworkError is defined and check its cases
rg "enum NetworkError" --type swift -A 20Repository: YAPP-Github/Bitnagil-iOS
Length of output: 1986
.invalidURL을 DomainError.requireRetry로 매핑하는 것을 재검토 필요
.invalidURL은 잘못된 URL 구성(개발자 실수)으로 인한 에러이므로 재시도해도 동일하게 실패합니다. .needRetry와 .emptyData는 일시적인 네트워크 문제일 수 있어 재시도가 의미 있지만, .invalidURL은 DomainError.business로 매핑하는 것이 더 적절합니다.
이 패턴이 OnboardingRepository의 세 메서드 모두에 적용되어 있으며, 코드베이스의 다른 모든 Repository(RoutineRepository, FileRepository, ReportRepository 등)에서도 동일하게 반복되고 있으므로 함께 수정이 필요합니다.
🔧 수정 제안
} catch let error as NetworkError {
switch error {
- case .needRetry, .invalidURL, .emptyData:
+ case .needRetry, .emptyData:
throw DomainError.requireRetry
default:
throw DomainError.business(error.description)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch error { | |
| case .needRetry, .invalidURL, .emptyData: | |
| throw DomainError.requireRetry | |
| default: | |
| throw DomainError.business(error.description) | |
| } | |
| switch error { | |
| case .needRetry, .emptyData: | |
| throw DomainError.requireRetry | |
| default: | |
| throw DomainError.business(error.description) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/DataSource/Sources/Repository/OnboardingRepository.swift` around
lines 24 - 29, The switch currently maps `.invalidURL` to
DomainError.requireRetry along with `.needRetry` and `.emptyData`; change the
mapping so only transient errors (`.needRetry` and `.emptyData`) throw
DomainError.requireRetry, while `.invalidURL` should throw
DomainError.business(error.description). Update this logic in the switch blocks
inside OnboardingRepository's three methods (the switch handling `error` that
currently lists `.needRetry, .invalidURL, .emptyData`) and apply the same fix
consistently across other repositories that repeat this pattern (e.g.,
RoutineRepository, FileRepository, ReportRepository) so `.invalidURL` is treated
as a business error while retryable cases remain requireRetry.
Comment on lines
+22
to
+27
| switch error { | ||
| case .needRetry, .invalidURL, .emptyData: | ||
| throw DomainError.requireRetry | ||
| default: | ||
| throw DomainError.business(error.description) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Projects/DataSource/Sources/Repository/RecommendedRoutineRepository.swiftRepository: YAPP-Github/Bitnagil-iOS
Length of output: 2549
🏁 Script executed:
rg -n "\.needRetry.*\.invalidURL.*\.emptyData" --type swiftRepository: YAPP-Github/Bitnagil-iOS
Length of output: 2614
🏁 Script executed:
rg -n "case \.needRetry" --type swift -A 3Repository: YAPP-Github/Bitnagil-iOS
Length of output: 10040
.invalidURL, .emptyData를 requireRetry로 매핑하지 않기
.needRetry는 타임아웃/연결 끊김 같은 일시적 오류이므로 재시도가 의미 있습니다. 그러나 .invalidURL은 영구적 오류로 재시도해도 항상 실패하고, .emptyData도 일시적 오류가 아닐 가능성이 높습니다. 현재 이 경우들을 DomainError.requireRetry로 매핑하면 사용자에게 오도하는 "재시도" 프롬프트를 표시하게 됩니다.
이 패턴은 RecommendedRoutineRepository 외에도 LocationRepository, RoutineRepository, ReportRepository, OnboardingRepository, FileRepository, EmotionRepository 등 9개 파일의 21개 위치에서 동일하게 사용되고 있어 프로젝트 전반에 걸친 개선이 필요합니다.
제안된 수정
} catch let error as NetworkError {
switch error {
- case .needRetry, .invalidURL, .emptyData:
+ case .needRetry:
throw DomainError.requireRetry
default:
throw DomainError.business(error.description)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch error { | |
| case .needRetry, .invalidURL, .emptyData: | |
| throw DomainError.requireRetry | |
| default: | |
| throw DomainError.business(error.description) | |
| } | |
| switch error { | |
| case .needRetry: | |
| throw DomainError.requireRetry | |
| default: | |
| throw DomainError.business(error.description) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/DataSource/Sources/Repository/RecommendedRoutineRepository.swift`
around lines 22 - 27, The switch incorrectly maps .invalidURL and .emptyData to
DomainError.requireRetry (only .needRetry should trigger retries); update the
switch in RecommendedRoutineRepository (and the same pattern in
LocationRepository, RoutineRepository, ReportRepository, OnboardingRepository,
FileRepository, EmotionRepository, etc.) so that case .needRetry throws
DomainError.requireRetry, while .invalidURL and .emptyData (and other
non-transient cases) throw DomainError.business(error.description) (preserving
the existing default behavior) to avoid prompting retries for permanent errors.
Comment on lines
+27
to
+37
| func handleNetworkError(_ error: Error, retryAction: @escaping () -> Void) { | ||
| guard | ||
| let domainError = error as? DomainError, | ||
| domainError == .requireRetry | ||
| else { | ||
| // 네트워크 에러 문제가 아닐 경우 | ||
| return | ||
| } | ||
|
|
||
| networkErrorActionSubject.send(retryAction) | ||
| } |
There was a problem hiding this comment.
DomainError.requireRetry가 아닌 에러가 무시됩니다.
handleNetworkError는 DomainError.requireRetry가 아닌 경우 조용히 return합니다. ViewModel의 catch 블록에서 이 메서드에만 의존하면 DomainError.business나 DomainError.unknown 같은 에러가 사용자에게 아무런 피드백 없이 무시됩니다.
예시 (ReportHistoryViewModel):
} catch {
networkRetryHandler.handleNetworkError(error) { [weak self] in
self?.fetchReports()
}
}최소한 로깅을 추가하거나, 호출부에서 비-네트워크 에러에 대한 별도 처리를 보장해야 합니다.
🛡️ 로깅 추가 제안
func handleNetworkError(_ error: Error, retryAction: `@escaping` () -> Void) {
guard
let domainError = error as? DomainError,
domainError == .requireRetry
else {
// 네트워크 에러 문제가 아닐 경우
+ BitnagilLogger.log(logType: .error, message: "Non-retry error occurred: \(error)")
return
}
networkErrorActionSubject.send(retryAction)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/Presentation/Sources/Common/Protocol/NetworkRetryCapable.swift`
around lines 27 - 37, handleNetworkError currently returns silently for
non-DomainError.requireRetry errors, causing DomainError.business/unknown to be
ignored; update handleNetworkError to log or forward non-retry errors instead of
silently returning: inside the else branch (where you now comment "// 네트워크 에러
문제가 아닐 경우") call a logger (or send the error to an existing error reporting
subject) and/or expose outcome to callers (e.g., return a Bool handled flag or
add a nonRetryErrorSubject) so ViewModels can surface or handle
DomainError.business/unknown; refer to handleNetworkError,
DomainError.requireRetry, and networkErrorActionSubject when making the change.
Comment on lines
166
to
173
| } catch { | ||
| BitnagilLogger.log(logType: .error, message: "\(error.localizedDescription)") | ||
| registerRoutineResultSubject.send(false) | ||
|
|
||
| networkRetryHandler.handleNetworkError(error) { [weak self] in | ||
| self?.registerRecommendedRoutine() | ||
| } | ||
| } |
There was a problem hiding this comment.
네트워크 에러 시 registerRoutineResultSubject.send(false)와 네트워크 에러 뷰가 동시에 트리거됩니다.
DomainError.requireRetry인 경우, Line 168에서 registerRoutineResultSubject.send(false)로 실패 신호를 보내고, Line 170에서 네트워크 에러 뷰도 표시합니다. VC에서 registerRoutineResultPublisher의 false 값에 대해 별도 에러 처리(토스트, 얼럿 등)를 하고 있다면 네트워크 에러 뷰와 중복 표시될 수 있습니다.
네트워크 재시도 대상 에러일 경우 send(false)를 건너뛰거나, VC 측에서 중복 처리가 없는지 확인해 주세요.
#!/bin/bash
# VC에서 registerRoutineResultPublisher를 어떻게 처리하는지 확인
fd "ResultRecommendedRoutineViewController" --extension swift --exec cat {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Projects/Presentation/Sources/ResultRecommendedRoutine/ViewModel/ResultRecommendedRoutineViewModel.swift`
around lines 166 - 173, In ResultRecommendedRoutineViewModel, avoid emitting
registerRoutineResultSubject.send(false) for retryable network errors
(DomainError.requireRetry) so the VC doesn't get a duplicate error UI; modify
the catch in registerRecommendedRoutine() to detect DomainError.requireRetry (or
use networkRetryHandler.isRetryable(error)) and only call
registerRoutineResultSubject.send(false) when the error is not retryable,
leaving networkRetryHandler.handleNetworkError(error) {
self?.registerRecommendedRoutine() } to handle retryable cases; reference
registerRoutineResultSubject.send(false),
networkRetryHandler.handleNetworkError(error){...}, DomainError.requireRetry,
and registerRoutineResultPublisher so VC receives a single error signal.
choijungp
approved these changes
Feb 18, 2026
Contributor
choijungp
left a comment
choijungp
left a comment
There was a problem hiding this comment.
그르니까 제가 이해한 바로는
- ViewModel ( .. Repository .. Domain)에서 네트워크 유실 에러가 발생
- ViewModel의 networkRetryHandler를 통해 해당 메서드를 retryAction으로 send !!
- ViewController (BaseViewController) 에서는 bindNetworkError를 통해 해당 네트워크 에러 Subject를 구독하다가
- retryAction이 전달되면 networkError 뷰를 show 하고, networkError 뷰의 onRetry 함수를 저장
- 요청 성공시 clearRetryState를 통해 etryAction을 nil로 보내서 hide
이것이 마즐까요 ?? ?????
그르티만 ~ .. ViewModel에서 너무 중복 코드가 생기는 것이 고민이다 ...
그렇다면 !!!!
현재 네트워크 요청을 보내는 ViewModel들이 중복적으로 networkRetryHandler를 갖고 있으니까
해당 프로퍼티를 ViewModel protocol에 정의해두고,
extension ViewModel을 통해 중복되는 함수의 행동을 정의하는건 어떤가욤 ??
예를 들어
public extension ViewModel {
func executeWithRetry(
action: @escaping () async throws -> Void,
retryAction: @escaping () -> Void
) {
Task {
do {
try await action()
networkRetryHandler.clearRetryState()
} catch {
networkRetryHandler.handleNetworkError(error, retryAction)
}
}
}
}
// HomeViewModel.swift
private func loadNickname() {
executeWithRetry(
action: { [weak self] in
guard let self else { return }
let nickname = try await self.userDataUseCase.loadNickname()
self.nicknameSubject.send(nickname)
},
retryAction: { [weak self] in
self?.loadNickname()
}
)
}이렇게 ... ???
다만 수정할 부분이 많을 것 같아 위의 방법이 괜찮다면 ViewModel에 추가 후 각자 수정하는 것도 좋은 방법이라고 생각합니다 !!!!
띵푸루부 완전 척척박사 띵 ~
감사합니다 !!!
|
|
||
| public enum DomainError: Error, Equatable { | ||
| case requireRetry | ||
| case business(String) |
Contributor
There was a problem hiding this comment.
요 business 에러는 무슨 상황일 때를 뜻하는지 궁금함니다 !!
Collaborator
Author
There was a problem hiding this comment.
날카롭게 바로 캐치하셨군요!!! 해당 부분 관련해서 잊어버리지 않게 저희 노션에 데스노트로 추가해 두었습니다.
일단 도메인 레이어에 Error를 정의한 이유부터 말씀드려야 할 것 같습니다!
api를 쐈을때 크게 3가지의 에러가 발생할 수 있다고 생각하는데요~
- 네트워크 유실 등 연결 문제
- 클라이언트의 문제 (400 ~)
- 서버 에러 (500~)
그리고 이번에 디자인에 새로 추가된 재시도 화면은 이 중에서 1번 문제만 핸들링한다고 생각했습니다. 2, 3번 에러는 발생 시에 그에 맞는 화면이나 alert를 띄워줘야한다고 생각했기 때문입니다.
그렇다면, presentation 단에서는 위 1, 2, 3에 해당하는 에러 중에 어떤 에러가 발셍했는지 알 수 있어야 합니다. 다만 presentation layer는 datasource layer를 알 수 없기 때문제, domain 단에 error를 정의할 필요가 있었습니다.
Domain 단에서 발생할 수 있는 에러를 좀 더 세분화해서 정의해야 하는데, 사실 이번 pr을 좀 더 빨리 쳐내고 싶은 마음이 크기도 하고,, error 정의와 처리 관련해서는 새로운 PR로 분리하는 것이 좋을 것 같다는 생각도 했습니다.
때문에 우선 현재 필요한 네트워크 연결 문제: 재시도 필요 를 뜻하는 requireRetry 만 정의하고, 발생할 수 있는 나머지 에러들(서버 에러나 도메인 layer에서 발생가능한 기타 비즈니스 로직 에러)은 비즈니스 에러로 일단 퉁!! 쳐서 구현해두었습니다.
Collaborator
Author
|
@choijungp
해당 구조로 구현하게 되면 반복 코드를 줄일 수 있고 좀 더 직관적이고 간단한 구조로 구현이 가능할 것이라 생각했죠! 다만 이렇게 하게 되면, 구조가 비효율적으로 변한 감이 없지 않아 있는것 같아,, 어떤 방법이 더 괜찮을 지 편하게 말씀주시면 감사하겠습니다!! |
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.
🌁 Background
📱 Screenshot
👩💻 Contents
NetworkRetryCapable구현BaseViewController에서 네트워크 재시도 View를 띄울 수 있도록 구현📝 Review Note
구조 설명
각 링크를 클릭하면 해당 파일로 이동합니다
NetworkRetryCapable(네트워크 재시도 protocol)
ViewModel외부 api를 호출하는 메서드 내부에서 네트워크 timeout이나 lost connection 과 같은 Error가 발생하는 경우,handleNetworkError를 통해 오류가 발생한viewModel의 메서드를networkErrorActionSubject로 방출합니다.네트워크 재시도 View
BaseViewController에 등록되어있습니다.configureLayout과configureAttribute에서 super인BaseViewController의configureAttribute와configureLayout을 호출하도록 수정하였습니다.onRetry클로저에 재시도할 메서드가 들어가게 되고, 사용자가 재시도 버튼을 누르면 해당 메서드를 실행하게 됩니다.BaseViewController
빛나길 VC가 상속하는 BaseViewController의 핵심 변경 사항은 아래 두가지 메서드가 추가됐다는 점입니다.
bindNetworkError가 핵심 함수입니다. 각 VC의bind함수 내에서, viewModel에 추후 추가될 networkError 관련 publisher를 넣어주면 됩니다.사용 방법
ViewModel의Output에networkErrorPublisher추가ViewModel 내부에
NetworkRetryHandler를 추가재시도가 필요한 메서드의 catch 문에서, handleNetworkError 를 실행합니다.
VC에
bindNetworkError()추가viewModel의networkErrorPublisher를bindNetworkError에 넣어 호출합니다.개선해야할 점
Summary by CodeRabbit
릴리스 노트
새로운 기능
버그 수정