Feature/#99 - 사이드바 Notice 필터 탭 sorting 수정 - #100
Conversation
.notice가 두 곳(MainFeature, SidebarClient+Live)에서 .all로 매핑된 TODO 상태였다. 도메인에 SecretQuery.Collection.notice(referenceDate:) 케이스를 신설해 실제로 연결했다. .expired를 재사용하지 않고 새 케이스를 만든 이유는 SecretListView가 collection 타입 하나로 화면 정체성(제목·섹션·정렬 표시 여부)을 전부 결정하기 때문이다 — .expired를 재사용하면 제목이 "Expired"로 뜨고 원치 않는 섹션 헤더까지 딸려온다. 새 케이스를 두면 titleText/list/ showsSort의 기존 exhaustive switch가 컴파일러 도움으로 자연히 구분된다. 판정 규칙: deletedAt == nil && expiresAt > referenceDate && expiresAt <= referenceDate + noticeWindowDays(7일). 이미 지난 것과 만료일 없는 것은 제외 — 목록 행 배지의 upcoming window와 같은 기준을 써서 사이드바 카드 숫자와 배지가 뜨는 시크릿 집합이 어긋나지 않게 했다.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 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 |
doyeonk429
left a comment
There was a problem hiding this comment.
#99 Notice 탭 연결 리뷰입니다. 머지 전 처리를 권하는 건 P0 하나이고, P1은 이 PR에서 같이 정리하면 좋고, P2·P3는 방향만 정해두면 후속으로 넘겨도 됩니다.
새 케이스를 만들어 expiringWindow 트릭 재사용을 피한 판단, default: → 명시 케이스 전환, "왜"를 설명하는 주석은 그대로 좋았습니다. 위험은 로직이 아니라 검증 경로가 실제 실행 경로를 지나지 않는다는 쪽에 있습니다.
| case let .notice(referenceDate): | ||
| // 이미 지났거나(≤ referenceDate) window를 벗어난(> windowEnd) 것은 제외한다. | ||
| // 만료일이 없는 Secret은 `.distantFuture`로 치환되어 두 조건 다 자연히 걸러진다. | ||
| let neverExpires = Date.distantFuture | ||
| let windowEnd = referenceDate.addingTimeInterval( | ||
| TimeInterval(SecretQuery.Collection.noticeWindowDays) * 86_400 | ||
| ) |
There was a problem hiding this comment.
[P0] .notice predicate가 실제 SwiftData에서 실행 검증되지 않았습니다
문제
이 #Predicate는 컴파일이 통과해도 fetch 시점에 실패할 수 있는 종류의 코드인데, 지금 저장소에 그걸 잡아줄 경로가 없습니다.
Projects/DVData/Tests/에는.gitkeep하나뿐 — DVData에 테스트 타겟이 없습니다.- 저장소 전체에서 테스트용
ModelContainer를 만드는 곳이 없습니다. PR 본문의 DVDomain 182 tests는InMemorySecretRepository(손으로 짠 replica)를 검증할 뿐 SQL 번역과는 무관합니다. - PR 본문상 실제 앱에서 Notice 탭을 눌러본 기록도 없습니다.
바로 아래 .expired case의 주석이 같은 위험을 경고하고 있고("...SwiftData가 SQL로 번역하지 못해 fetch 시점에 실패한다"), 이 predicate는 이 파일에서 한 표현식에 ??를 두 번 쓰는 첫 사례입니다(L109-110). BUILD SUCCEEDED는 이 조합의 번역 가능 여부를 보증하지 않습니다.
해결방안 제안
(A) 최소 — 머지 전 앱 실행 → Notice 탭 목록이 뜨는지, 사이드바 카드 숫자와 목록 개수가 같은지 확인하고 결과만 코멘트로 남기기.
(B) 권장 — 비어 있는 DVData/Tests에 in-memory 테스트 추가:
let container = try ModelContainer(
for: SwiftDataModel.Secret.self,
configurations: .init(isStoredInMemoryOnly: true)
)
// 이미 지남 / window 이내 / 경계 / window 밖 / expiresAt == nil 5건 삽입 후
let descriptor = SecretFetchDescriptorBuilder.make(
from: SecretQuery(collection: .notice(referenceDate: now))
)
#expect(try container.mainContext.fetch(descriptor).count == 2)(B)로 가면 리뷰 가이드에서 질문하신 "replica와 실제 predicate가 어긋나지 않는지"도 같이 잡힙니다.
| /// Notice에 담을 "만료 임박" 기간(일). 목록 행 배지의 upcoming window(7일)와 같은 기준을 써야 | ||
| /// 사이드바 카드 숫자와 배지가 뜨는 시크릿 집합이 어긋나지 않는다. | ||
| public static let noticeWindowDays = 7 |
There was a problem hiding this comment.
[P1] "7일"이 두 모듈에 따로 존재하고, 새 테스트가 그 불변식을 검증하지 못합니다
문제
SecretExpiryStatus는 파일 주석에 "만료 임박 표기 정책의 단일 정의부"라고 선언돼 있는데, 이 상수가 두 번째 7이 됐습니다. 현재 7일 표현이 리터럴 2개 · 경계 규칙 3종으로 갈라져 있습니다.
| 위치 | 값 | 상단 경계 |
|---|---|---|
SecretExpiryStatus.upcomingWindow |
7 * 86_400 |
<= ref+7d 포함 |
noticeWindowDays (이 PR) |
7 |
<= ref+7d 포함 |
ExpiryBucket.within7Days |
7 * 86_400 하드코딩 |
< ref+7d 제외 |
정확히 ref+7d에 만료되는 시크릿은 Notice 탭엔 뜨지만, Expired 탭에서는 "Expires in 7 days"가 아니라 "Expires in 30 days" 섹션에 들어갑니다.
또 새로 추가된 SecretQueryTests.noticeWindowDaysMatchesUpcomingWindow는 이름이 주장하는 불변식을 검증하지 못합니다. DVDomain은 DVPresentation을 import할 수 없어 구조적으로 불가능하고, 지금은 7 == 7 동어반복이라 upcomingWindow를 14로 바꿔도 통과합니다.
해결방안 제안
DVPresentation → DVDomain 의존이 이미 있으니 이 방향만 컴파일됩니다.
// SecretExpiryStatus.swift
static let upcomingWindow: TimeInterval =
TimeInterval(SecretQuery.Collection.noticeWindowDays) * secondsPerDay검증 테스트는 DVPresentation 쪽으로:
#expect(SecretExpiryStatus.upcomingWindow
== TimeInterval(SecretQuery.Collection.noticeWindowDays) * 86_400)ExpiryBucket.contains의 7 * 86_400도 같이 갈아끼우면 리터럴이 하나로 줄고 경계 규칙도 통일됩니다.
| /// 만료 배지(critical/upcoming)가 붙는 대상만 모은 컬렉션. 이미 만료된 것은 제외한다 — | ||
| /// Expired 탭이 "이미 지남"을 전담하므로 여기서까지 중복해 보여줄 이유가 없다. | ||
| case notice(referenceDate: Date) |
There was a problem hiding this comment.
[P2] doc comment의 근거가 실제 동작과 어긋납니다 — Notice는 Expired의 부분집합입니다
문제
두 가지가 사실과 다릅니다.
1. "만료 배지(critical/upcoming)가 붙는 대상만 모은 컬렉션"
이미 만료된 시크릿도 .critical 배지가 붙지만(SecretExpiryStatus가 음수 잔여기간을 critical로 판정) Notice에서는 제외됩니다. L42-43의 "사이드바 카드 숫자와 배지가 뜨는 시크릿 집합이 어긋나지 않게"도 같은 이유로 성립하지 않습니다.
2. "Expired 탭이 '이미 지남'을 전담하므로 중복해 보여줄 이유가 없다"
Expired 쪽은 이미 Notice 전체를 포함하고 있습니다.
- 사이드바 Expired 카드 =
.expiringWindow(from:)= 이미 지남 + 향후 30일 - Expired 탭 리스트 = 같은 30일 window를
Expired / 7일 / 30일섹션으로 표시
즉 Notice(7일) ⊂ Expired(30일) 이고, 중복 회피가 한쪽 방향에만 적용된 셈입니다.
해결방안 제안
동작 자체는 제품 결정 사항이라 판단은 작성자/기획 몫입니다. 다만 근거가 틀린 채로 남으면 다음 사람이 이걸 "버그"로 보고 고칠 위험이 있습니다.
의도가 "Notice = 아직 안 지났지만 곧 지날 것, Expired와는 관점이 다른 뷰" 라면 주석을 그렇게 다시 써주세요. 반대로 Notice ⊄ Expired 로 만들 생각이라면 후속 이슈로 트래킹 부탁드립니다.
| case .notice(let referenceDate): | ||
| let windowEnd = referenceDate.addingTimeInterval( | ||
| TimeInterval(SecretQuery.Collection.noticeWindowDays) * 86_400 | ||
| ) | ||
| filtered = [Secret].preview.filter { | ||
| guard let expiresAt = $0.expiresAt else { return false } | ||
| return $0.deletedAt == nil && expiresAt > referenceDate && expiresAt <= windowEnd | ||
| } |
There was a problem hiding this comment.
[P2] window 계산이 3곳에 복붙돼 있습니다 (리뷰 가이드 질문 항목)
문제
동일한 계산이 세 군데에 있습니다.
SecretFetchDescriptorBuilder.predicateL104-106InMemorySecretRepository.matchesCollection(DVDomain 테스트 지원)- 여기
dummyClientL70-72
해결방안 제안
세 호출부 모두 windowEnd를 #Predicate 클로저 바깥에서 계산하므로, expiringWindow(from:)이 이미 자리잡은 곳에 static 헬퍼를 두면 그대로 대체됩니다.
// SecretQuery.Collection
public static func noticeWindowEnd(from referenceDate: Date) -> Date {
referenceDate.addingTimeInterval(TimeInterval(noticeWindowDays) * 86_400)
}86_400 리터럴 3개가 사라집니다. 다만 판정식 자체를 도메인 헬퍼로 빼는 건 권하지 않습니다 — #Predicate 안에 클로저 호출을 넣을 수 없어 predicate는 어차피 따로 써야 하고, 반쪽짜리 공유가 더 헷갈립니다.
리뷰 가이드에서 물어보신 noticeWindowDays 위치는 지금 자리가 맞다고 봅니다. expiringSoonWindowDays 바로 옆이고, #97 머지 후 두 상수를 함께 옮기는 편이 diff가 깔끔합니다.
| ) | ||
| return #Predicate<SwiftDataModel.Secret> { secret in | ||
| secret.deletedAt == nil && | ||
| (secret.expiresAt ?? neverExpires) > referenceDate && |
There was a problem hiding this comment.
[P3] 하한 경계만 > 라서 어디에도 안 잡히는 구간이 생깁니다
문제
같은 파일 안에서 하한 비교가 갈립니다.
.all/.liked:expiresAt >= referenceDate(정각 = 아직 유효).expired:expiresAt < referenceDate.notice(이 줄):expiresAt > referenceDate
expiresAt == referenceDate인 시크릿은 Notice에도 Expired에도 없고 All에만 남습니다. ExpiryBucket.within7Days도 하한이 >= referenceDate라 여기만 규칙이 다릅니다.
실무 확률은 0에 가깝지만, "Notice = 아직 안 지난 것"이라는 규칙을 .all과 맞추려면 >=가 맞습니다.
해결방안 제안
(secret.expiresAt ?? neverExpires) >= referenceDate &&InMemorySecretRepository와 dummyClient의 같은 비교도 함께 맞춰주세요.
참고로 리뷰 가이드에서 물어보신 ?? neverExpires 관용구 자체는 기존 .expired와 일관되고 문제없습니다 — 연산자 한 글자만 다릅니다.
Notice/Expired 7일 경계 불일치와 하한 비교(>) 누락 구간을 >=로 통일하고, window 계산 중복을 SecretQuery.Collection.noticeWindowEnd로 정리했다. DVData에 테스트 타겟을 추가해 .notice predicate를 실제 ModelContainer로 fetch 검증하고, upcomingWindow가 noticeWindowDays에서 파생되게 했다.
✨ What's this PR?
📌 관련 이슈 (Related Issue)
🧶 주요 변경 내용 (Summary)
사이드바 Notice 필터 탭을 실제 데이터에 연결
.notice가 두 곳(MainFeature.makeSecretListState,SidebarClient+Live.SidebarFilter.collection)에서.all로 매핑된 TODO 상태였습니다. Notice 탭을 눌러도 All과 똑같은 목록이 뜨고, 사이드바 카드 숫자도 실제 의미가 없었습니다..expired를 재사용하지 않고 새 케이스(SecretQuery.Collection.notice)를 만든 이유SecretListView가collection타입 하나로 화면 정체성(제목·섹션·정렬 표시 여부)을 전부 결정하는 구조입니다..expired에 window만 밀어넣는 기존 트릭(expiringWindow(from:))을 재사용하면 뷰가 "이건 Expired 탭"으로 착각해 제목이 "Expired"로 뜨고 원치 않는 섹션 헤더까지 딸려옵니다. 새 케이스를 두면titleText/showsSort/contextMenuItems의 기존 exhaustive switch가 컴파일러 도움으로 자연히 구분됩니다.판정 규칙
이미 지난 것과 만료일 없는 것은 제외했습니다. 목록 행 배지(critical/upcoming)의 upcoming window와 같은 7일 기준을 써서, 사이드바 카드 숫자와 실제로 배지가 뜨는 시크릿 집합이 어긋나지 않게 했습니다.
영향 범위
SecretQuery.Collection에notice(referenceDate:)케이스 +noticeWindowDays상수 추가 (DVDomain)SecretFetchDescriptorBuilder.predicate,InMemorySecretQueryFilter.matchesExpiry에.notice분기 추가 (DVData)MainFeature,SidebarClient+Live의 TODO 두 곳을 실제 매핑으로 교체SecretListFeature.State.query,SecretListView(titleText/showsSort/contextMenuItems/미리보기)에.notice반영SecretClient프리뷰(dummyClient)에도.notice필터링 추가📸 스크린샷 (Optional)
🧪 테스트 / 검증 내역
xcodebuild testDVDomain — 182 tests passed (.notice판정 로직, window 경계값 포함)xcodebuild testDVPresentation — 161 tests passed (.noticequery가 collection을 그대로 쓰고 정렬을 강제하는지 포함)xcodebuild buildDevault(앱 전체) — BUILD SUCCEEDED💬 기타 공유 사항
.notice는 항상 만료 임박 순으로 고정 정렬되며(Expired 탭과 동일한 패턴), 사용자가 정렬을 바꿀 UI는 없습니다(showsSort = false). 다른 정렬 옵션이 필요하면 후속 이슈로 넘기겠습니다.SecretExpiryStatus)의 upcoming 판정과 동일한 경계 규칙입니다.develop기준으로 독립적으로 작업했습니다.#97(SecretExpiryPolicy 도입)이 먼저 머지되면,noticeWindowDays를 그쪽 상수로 옮기는 정리가 후속으로 필요할 수 있습니다 — 지금은SecretQuery.Collection안에 로컬 상수로 뒀습니다(기존expiringSoonWindowDays와 같은 자리, 같은 스타일).🙇🏻♀️ 리뷰 가이드 (선택)
SecretQuery.swift의noticeWindowDays상수 위치 —#97이 머지된 후SecretExpiryPolicy로 옮길지, 지금 이대로 둘지SecretFetchDescriptorBuilder.predicate의.noticecase —?? neverExpires관용구로 두 경계(이미 지남/window 밖)를 한 번에 걸러내는 방식이 기존 코드 스타일과 일관되는지InMemorySecretRepository.matchesCollection(DVDomain 테스트 지원 코드)에도 같은 판정 로직을 복제했습니다 — 실제 SwiftData predicate와 별개 구현이라 두 곳이 어긋나지 않는지 확인 부탁드립니다