[#289] Todo 카테고리를 유저가 CRUD 할 수 있도록 개선한다#337
Conversation
opficdev
commented
Mar 30, 2026
opficdev
commented
- closed Todo 카테고리를 유저가 CRUD 할 수 있도록 개선한다 #289
… 기존 카테코리는 두 종류를 아우르는 형태로 구성
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| switch category { | ||
| case .decoded(let category): | ||
| todoCategory = category | ||
| case .raw(let category): | ||
| guard let systemTodoCategory = SystemTodoCategory(rawValue: category) else { | ||
| throw DataError.invalidData("TodoResponse.category is invalid: \(category)") | ||
| } | ||
| todoCategory = .system(systemTodoCategory) | ||
| } |
There was a problem hiding this comment.
toDomain() 메서드에서 .raw 케이스를 처리하는 방식이 잠재적인 버그를 유발할 수 있습니다. 현재 코드는 raw value를 SystemTodoCategory로만 변환하려고 시도하며, 사용자 정의 카테고리 ID가 들어올 경우 invalidData 오류를 발생시킵니다.
Repository 단에서 미리 카테고리를 resolve하고 있지만, 이 toDomain 메서드가 다른 곳에서 resolve되지 않은 DTO와 함께 호출될 경우 예기치 않은 동작을 일으킬 수 있습니다.
.raw 케이스를 받았을 때 오류를 던져서, 항상 Repository에서 카테고리를 resolve한 후에 toDomain()을 호출하도록 강제하는 것이 더 안전한 설계일 것 같습니다. PushNotificationMapping.swift 파일의 toDomain() 메서드에도 동일한 문제가 있습니다.
switch category {
case .decoded(let category):
todoCategory = category
case .raw(let rawValue):
throw DataError.invalidData("Unresolved raw category value found: \(rawValue). This must be resolved in the repository before calling toDomain().")
}| receiveValue: { [weak self] response in | ||
| guard let self else { return } | ||
|
|
||
| Task { | ||
| do { | ||
| let preferences = try await self.todoCategoryService.fetchPreferences() | ||
| let userTodoCategories: [UserTodoCategory] = preferences.compactMap { preference in | ||
| guard case .user(let userTodoCategory) = preference.category else { | ||
| return nil | ||
| } | ||
|
|
||
| return userTodoCategory | ||
| } | ||
|
|
||
| let responses = try response.items.map { | ||
| try self.resolve($0, userTodoCategories: userTodoCategories) | ||
| } | ||
|
|
||
| let page = try PushNotificationPageResponse( | ||
| items: responses, | ||
| nextCursor: response.nextCursor | ||
| ).toDomain() | ||
|
|
||
| subject.send(page) | ||
| } catch { | ||
| subject.send(completion: .failure(error)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
requestNotifications 메서드와 observeNotifications 메서드 내부에 카테고리 preferences를 가져와 resolve를 통해 페이지를 만드는 로직이 중복되고 있습니다. 이 로직을 별도의 private 헬퍼 메서드로 추출하면 코드 중복을 줄이고 가독성을 높일 수 있을 것 같습니다.
예를 들어, 다음과 같은 헬퍼 메서드를 만들 수 있습니다.
private func resolvePage(from response: PushNotificationPageResponse, with preferences: [TodoCategoryPreference]) throws -> PushNotificationPage {
let userTodoCategories: [UserTodoCategory] = preferences.compactMap { preference in
guard case .user(let userTodoCategory) = preference.category else {
return nil
}
return userTodoCategory
}
let responses = try response.items.map {
try resolve($0, userTodoCategories: userTodoCategories)
}
return try PushNotificationPageResponse(
items: responses,
nextCursor: response.nextCursor
).toDomain()
}| let category: TodoCategory | ||
| switch response.category { | ||
| case .decoded(let decodedCategory): | ||
| category = decodedCategory | ||
| case .raw(let value): | ||
| throw DataError.invalidData("TodoReferenceResponse.category is invalid: \(value)") | ||
| } |
There was a problem hiding this comment.
resolve 함수를 호출한 후에는 response.category가 항상 .decoded 케이스가 되므로, switch 구문은 불필요해 보입니다. .raw 케이스는 실행될 수 없는 코드입니다.
guard case let .decoded(category) = response.category를 사용하여 코드를 더 간결하게 만들 수 있습니다.
guard case .decoded(let category) = response.category else {
throw DataError.invalidData("TodoReferenceResponse.category was not resolved: \(response.id)")
}| func resolve( | ||
| _ response: TodoResponse, | ||
| userTodoCategories: [UserTodoCategory] | ||
| ) throws -> TodoResponse { | ||
| let id: String | ||
| switch response.category { | ||
| case .raw(let value): | ||
| id = value | ||
| case .decoded: | ||
| return response | ||
| } | ||
|
|
||
| let category: TodoCategory | ||
| if let systemTodoCategory = SystemTodoCategory(rawValue: id) { | ||
| category = .system(systemTodoCategory) | ||
| } else if let userTodoCategory = userTodoCategories.first(where: { | ||
| $0.id == id | ||
| }) { | ||
| category = .user(userTodoCategory) | ||
| } else { | ||
| throw DataError.invalidData("TodoResponse.category is invalid: \(id)") | ||
| } | ||
|
|
||
| return TodoResponse( | ||
| id: response.id, | ||
| isPinned: response.isPinned, | ||
| isCompleted: response.isCompleted, | ||
| isChecked: response.isChecked, | ||
| number: response.number, | ||
| title: response.title, | ||
| content: response.content, | ||
| createdAt: response.createdAt, | ||
| updatedAt: response.updatedAt, | ||
| completedAt: response.completedAt, | ||
| dueDate: response.dueDate, | ||
| tags: response.tags, | ||
| category: .decoded(category) | ||
| ) | ||
| } |
There was a problem hiding this comment.
TodoRepositoryImpl와 PushNotificationRepositoryImpl에 각각 구현된 resolve 함수의 로직이 거의 동일합니다. 카테고리 ID를 TodoCategory 도메인 객체로 변환하는 이 핵심 로직을 별도의 유틸리티나 해석기(resolver)로 추출하여 코드 중복을 줄이고 유지보수성을 높이는 것이 좋을 것 같습니다.
| var canSubmitUserCategory: Bool { | ||
| let trimmedCategoryName = state.category?.name.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| if trimmedCategoryName.isEmpty { | ||
| return false | ||
| } | ||
|
|
||
| return !SystemTodoCategory.allCases.contains { | ||
| $0.localizedName.localizedCaseInsensitiveCompare(trimmedCategoryName) == .orderedSame | ||
| } | ||
| } |
There was a problem hiding this comment.
canSubmitUserCategory 로직에서 시스템 카테고리와의 이름 중복은 확인하고 있지만, 사용자가 이미 생성한 다른 카테고리와의 이름 중복은 확인하지 않고 있습니다. 사용자가 동일한 이름의 카테고리를 여러 개 생성할 수 있게 되면 혼란을 줄 수 있습니다. 기존 사용자 카테고리 이름과도 중복되지 않는지 확인하는 로직을 추가하는 것을 고려해 보세요.
var canSubmitUserCategory: Bool {
guard let currentCategory = state.category else { return false }
let trimmedCategoryName = currentCategory.name.trimmingCharacters(in: .whitespacesAndNewlines)
if trimmedCategoryName.isEmpty {
return false
}
// 시스템 카테고리와 이름 중복 확인
if SystemTodoCategory.allCases.contains(where: { $0.localizedName.caseInsensitiveCompare(trimmedCategoryName) == .orderedSame }) {
return false
}
// 다른 사용자 카테고리와 이름 중복 확인
if state.preferences.contains(where: { preference in
guard case .user(let userCategory) = preference.category,
userCategory.id != currentCategory.id else {
return false
}
return userCategory.name.caseInsensitiveCompare(trimmedCategoryName) == .orderedSame
}) {
return false
}
return true
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
이번 풀 리퀘스트는 사용자 정의 할 일 카테고리 기능을 도입하고, 관련 데이터 모델 및 리포지토리 계층을 대대적으로 개편했습니다. 주요 변경 사항으로 TodoCategory를 시스템 및 사용자 유형으로 구분하고, 이를 지원하기 위한 TodoCategoryService와 관련 유스케이스들을 추가했습니다. 또한 Firebase Functions를 통해 카테고리 삭제 시 해당 할 일들을 '기타'로 이동시키는 동기화 로직을 구현했습니다. 리뷰 결과, 카테고리 로드 실패 시의 예외 처리 누락, 도메인 엔티티의 프로토콜 준수 변경에 따른 영향도 확인, 그리고 시스템 카테고리 이름 중복 체크 로직의 견고함 개선이 필요해 보입니다.
| do { | ||
| let preferences = try await fetchPreferencesUseCase.execute() | ||
| send(.setCategories(preferences.map(TodoCategoryPreferenceItem.init(from:)))) | ||
| } catch { } |
There was a problem hiding this comment.
fetchPreferencesUseCase.execute() 호출에서 발생하는 오류가 catch 블록에서 처리되지 않고 있습니다. 카테고리 정보를 가져오는 데 실패할 경우 사용자에게 알리거나 적절한 대체 동작을 수행해야 합니다. 현재는 오류가 발생해도 아무런 피드백이 없어 사용자 경험에 부정적인 영향을 줄 수 있습니다.
| } catch { } | |
| } catch { /* 오류 처리 */ } |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
이번 풀 리퀘스트는 사용자가 직접 카테고리를 정의하고 관리할 수 있는 커스텀 Todo 카테고리 기능을 도입합니다. 시스템 기본 카테고리 외에 사용자 정의 카테고리를 지원하기 위해 도메인 모델, 데이터 매퍼, 저장소 로직 및 관리 UI가 전반적으로 업데이트되었으며, 카테고리 변경 사항을 동기화하기 위한 Cloud Functions가 추가되었습니다. 주요 피드백으로는 Todo 및 TodoReference 구조체에서 Hashable 및 Equatable 프로토콜 준수가 제거됨에 따라 발생할 수 있는 부작용을 방지하기 위해 해당 프로토콜을 다시 추가하라는 제안이 있었습니다.
| import Foundation | ||
|
|
||
| struct Todo: Identifiable, Hashable { | ||
| struct Todo: Equatable { |
There was a problem hiding this comment.
Todo 구조체에서 Hashable 프로토콜 준수를 제거했습니다. Todo 객체가 Set의 요소나 Dictionary의 키로 사용되는 경우, 이 변경으로 인해 컴파일 오류나 런타임 문제가 발생할 수 있습니다. 기존 코드에서 Hashable에 의존하는 부분이 없는지 확인해야 합니다.
| struct Todo: Equatable { | |
| struct Todo: Equatable, Hashable { |
| import Foundation | ||
|
|
||
| struct TodoReferenceItem: Identifiable, Equatable { | ||
| struct TodoReference { |
There was a problem hiding this comment.
TodoReference 구조체에서 Equatable 프로토콜 준수를 제거했습니다. TodoReference는 값 타입이므로 Equatable을 유지하는 것이 비교 로직의 명확성과 안정성을 위해 권장됩니다.
| struct TodoReference { | |
| struct TodoReference: Equatable { |
1 participant
