[feat] Notification UI 구현 #57
Open
+243
−16
Conversation
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
📝 WalkthroughWalkthrough앱의 초기 화면을 알림 기능으로 변경하고, 새로운 알림 UI 컴포넌트(NotificationCell, NotificationView, NotificationViewController)를 추가하여 알림 목록을 표시합니다. 기존 UI 컴포넌트의 색상과 배경 스타일도 조정됩니다. Changes
Sequence Diagram(s)sequenceDiagram
participant AppCoordinator
participant NotificationViewController
participant NotificationView
participant UICollectionView
participant NotificationCell
AppCoordinator->>NotificationViewController: start(): 초기 root view controller 설정
activate NotificationViewController
NotificationViewController->>NotificationView: loadView(): mainView 로드
activate NotificationView
NotificationView->>UICollectionView: 초기화 및 레이아웃 설정
NotificationView->>NotificationCell: NotificationCell 등록
deactivate NotificationView
NotificationViewController->>NotificationViewController: viewDidLoad(): setupCollectionView()
NotificationViewController->>UICollectionView: dataSource, delegate 할당
UICollectionView->>NotificationCell: collectionView(_:cellForItemAt:)
activate NotificationCell
NotificationCell->>NotificationCell: configure(notification:) - 데이터 바인딩
deactivate NotificationCell
UICollectionView-->>NotificationViewController: 알림 항목 화면 표시
deactivate NotificationViewController
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 6
🤖 Fix all issues with AI agents
In `@SMASHING/Presentation/Home/Cell/MatchingCell.swift`:
- Line 63: The comment "emphasis 컬러 추가" does not match the code using
.Text.muted; confirm whether the intended color is an emphasis color or muted,
then either change the color in the setTitleColor call (the setTitleColor(...,
for: .normal) invocation that currently uses .Text.muted) to the intended
.Text.emphasis (or other correct token) or update the comment to accurately
describe that .Text.muted is being applied.
In `@SMASHING/Presentation/Notification/NotificationCell.swift`:
- Around line 86-91: In configure(notification: TempNotification) you’re using
hardcoded strings and an inverted background rule; replace the hardcoded "10:00
AM" and sample content with the passed notification.time (formatted as needed)
and notification.content by assigning to timeLabel.text and contentLabel.text
respectively, and flip the background logic so a new/unread notification
(notification.isNew) uses the highlighted surface color (e.g.,
.Background.surface) while read notifications use .clear; update references in
this method (typeLabel, timeLabel, contentLabel, backgroundColor, and the
TempNotification properties time/content/isNew) accordingly.
In `@SMASHING/Presentation/Notification/NotificationView.swift`:
- Around line 1-7: Update the top-of-file header comment to match the actual
filename: replace "NotificationViewController.swift" with
"NotificationView.swift" in the header comment of NotificationView.swift so the
file header accurately reflects the file name and avoids confusion.
In `@SMASHING/Presentation/Notification/NotificationViewController.swift`:
- Around line 43-45: The override of viewDidLoad() in NotificationViewController
is missing a call to super.viewDidLoad(), which prevents BaseViewController's
setup (view.backgroundColor, setUI(), setLayout()) from running; update the
NotificationViewController.override func viewDidLoad() to call
super.viewDidLoad() at the start of the method before calling
setupCollectionView() so BaseViewController.viewDidLoad() executes and the UI
initialization is applied.
- Around line 73-75: The comment in collectionView(_:layout:sizeForItemAt:) is
misleading because automatic sizing isn't enabled and the hardcoded
CGSize(width: collectionView.frame.width, height: 100) is used; either update
the comment to reflect this fixed-height behavior or enable automatic cell
sizing by configuring the collection view's flow layout (in NotificationView
where the layout is created) to set layout.estimatedItemSize =
UICollectionViewFlowLayout.automaticSize and then remove or adjust the
collectionView(_:layout:sizeForItemAt:) implementation so it no longer forces a
100pt height (or return UICollectionViewFlowLayout.automaticSize equivalents).
- Around line 59-71: In collectionView(_:cellForItemAt:) stop adding constraints
to cell.contentView with snp.makeConstraints (which causes duplicate constraint
conflicts on reuse); remove that block and rely on the item size provided in
collectionView(_:layout:sizeForItemAt:). Use the project's type-safe dequeue
extension instead of dequeueReusableCell(withReuseIdentifier:for:) when
obtaining NotificationCell (still configure with
dummyNotifications[indexPath.item] via NotificationCell.configure), and keep the
guard that casts to NotificationCell but avoid creating new constraints on
reuse.
🧹 Nitpick comments (3)
SMASHING/Presentation/Notification/NotificationCell.swift (1)
25-25: 사용되지 않는 프로퍼티입니다.
notification프로퍼티가 선언만 되어 있고 어디서도 사용되지 않습니다. 제거하거나configure()메서드에서 저장하도록 수정해 주세요.♻️ 수정 제안
- private var notification: TempNotification?또는
configure()메서드에서 저장이 필요한 경우:func configure(notification: TempNotification) { + self.notification = notification typeLabel.text = notification.type.displayTextSMASHING/Presentation/Notification/NotificationViewController.swift (1)
18-35: 더미 데이터가 반복되어 있습니다.동일한 4개의 알림이 4번 반복되어 있습니다. 테스트 목적이라면 괜찮지만, 다양한 케이스를 테스트하기 위해 더 다양한 데이터를 사용하는 것이 좋습니다.
SMASHING/Presentation/Notification/NotificationView.swift (1)
58-62: SwiftUI import 위치 조정을 권장합니다.Preview 관련 코드가 잘 작성되어 있으나,
import SwiftUI를 파일 상단의 다른 import 문들과 함께 배치하면 일관성이 향상됩니다.♻️ 리팩토링 제안
파일 상단 import 섹션에 추가:
import UIKit import SnapKit import Then +import SwiftUI그리고 하단에서 import 제거:
} -import SwiftUI `@available`(iOS 18.0, *) `#Preview` { NotificationViewController() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
SMASHING/Global/Coordinators/AppCoordinator.swiftSMASHING/Presentation/Home/Cell/MatchingCell.swiftSMASHING/Presentation/Notification/NotificationCell.swiftSMASHING/Presentation/Notification/NotificationView.swiftSMASHING/Presentation/Notification/NotificationViewController.swiftSMASHING/Presentation/NotificationViewController.swiftSMASHING/Presentation/ViewController.swift
💤 Files with no reviewable changes (1)
- SMASHING/Presentation/NotificationViewController.swift
🧰 Additional context used
🧬 Code graph analysis (2)
SMASHING/Presentation/Notification/NotificationViewController.swift (4)
SMASHING/Presentation/ViewController.swift (1)
viewDidLoad(15-20)SMASHING/Global/Base/BaseViewController.swift (1)
viewDidLoad(20-25)SMASHING/Global/Extensions/UICollectionView+.swift (1)
dequeueReusableCell(32-43)SMASHING/Presentation/Notification/NotificationCell.swift (1)
configure(86-91)
SMASHING/Presentation/Notification/NotificationView.swift (4)
SMASHING/Presentation/Notification/NotificationCell.swift (2)
setUI(56-58)setLayout(60-84)SMASHING/Global/Base/BaseUIView.swift (2)
setUI(28-28)setLayout(29-29)SMASHING/Global/Base/BaseViewController.swift (2)
setUI(34-34)setLayout(35-35)SMASHING/Global/Extensions/UIView+.swift (1)
addSubviews(12-16)
🔇 Additional comments (7)
SMASHING/Presentation/Home/Cell/MatchingCell.swift (2)
33-33: LGTM!
.Text.muted색상 변경이 적절하게 적용되었습니다.Also applies to: 56-56
39-40: LGTM!포맷팅 조정이 적절합니다.
SMASHING/Presentation/Notification/NotificationCell.swift (2)
13-19: LGTM!
TempNotification구조체가 알림에 필요한 속성들을 잘 정의하고 있습니다.
56-84: LGTM!UI 컴포넌트 설정과 SnapKit 레이아웃이 적절하게 구성되어 있습니다.
SMASHING/Presentation/ViewController.swift (1)
17-17: LGTM!배경색 변경이 적용되었습니다.
BaseViewController는.Background.canvas를 사용하고 이 클래스는.Background.overlay를 사용하는 것으로 의도적인 구분으로 보입니다.SMASHING/Global/Coordinators/AppCoordinator.swift (1)
20-26: 개발/테스트용 변경사항이 포함되어 있습니다.
NotificationViewController를 루트로 설정하고TabBarCoordinator가 주석 처리되어 있습니다. 이 변경사항이 개발 브랜치에 병합되어도 괜찮은지 확인해 주세요. 프로덕션에서는 기존 앱 플로우(TabBarCoordinator)가 유지되어야 할 것으로 보입니다.SMASHING/Presentation/Notification/NotificationView.swift (1)
13-56: LGTM!
NotificationView구현이 잘 되어 있습니다:
navigationBar클로저에서[weak self]캡처를 통해 순환 참조를 올바르게 방지하고 있습니다.- SnapKit을 사용한 레이아웃 제약 조건이 적절하게 설정되어 있습니다.
BaseUIView의setUI()및setLayout()메서드를 올바르게 오버라이드하고 있습니다.
notificationCollection이private이 아닌 것은NotificationViewController에서 delegate/dataSource 설정을 위해 의도된 것으로 보입니다.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| $0.setTitle("결과 작성하기", for: .normal) | ||
| $0.titleLabel?.font = .pretendard(.textMdM) | ||
| $0.setTitleColor(.Text.blue, for: .normal) //emphasis 컬러 추가 | ||
| $0.setTitleColor(.Text.muted, for: .normal) //emphasis 컬러 추가 |
There was a problem hiding this comment.
주석과 실제 코드가 일치하지 않습니다.
주석에는 "emphasis 컬러 추가"라고 되어 있지만, 실제 적용된 색상은 .Text.muted입니다. 주석을 수정하거나 의도한 색상이 맞는지 확인해 주세요.
📝 주석 수정 제안
- $0.setTitleColor(.Text.muted, for: .normal) //emphasis 컬러 추가
+ $0.setTitleColor(.Text.muted, for: .normal) // muted 컬러로 변경📝 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
| $0.setTitleColor(.Text.muted, for: .normal) //emphasis 컬러 추가 | |
| $0.setTitleColor(.Text.muted, for: .normal) // muted 컬러로 변경 |
🤖 Prompt for AI Agents
In `@SMASHING/Presentation/Home/Cell/MatchingCell.swift` at line 63, The comment
"emphasis 컬러 추가" does not match the code using .Text.muted; confirm whether the
intended color is an emphasis color or muted, then either change the color in
the setTitleColor call (the setTitleColor(..., for: .normal) invocation that
currently uses .Text.muted) to the intended .Text.emphasis (or other correct
token) or update the comment to accurately describe that .Text.muted is being
applied.
Comment on lines
+86
to
+91
| func configure(notification: TempNotification) { | ||
| typeLabel.text = notification.type.displayText | ||
| timeLabel.text = "10:00 AM" | ||
| contentLabel.text = "'와쿠와쿠' 님이 소중한 후기를 보내주셨어요! 지금 확인해 볼까요 를레히히" | ||
| backgroundColor = notification.isNew ? .clear : .Background.surface | ||
| } |
There was a problem hiding this comment.
configure() 메서드에서 전달된 데이터를 사용하지 않고 있습니다.
notification.time과 실제 컨텐츠 대신 하드코딩된 값이 사용되고 있습니다. 또한 isNew 상태에 따른 배경색 로직이 의도와 다를 수 있습니다 - 일반적으로 새로운(읽지 않은) 알림은 강조 배경을, 읽은 알림은 기본 배경을 사용합니다.
🐛 수정 제안
func configure(notification: TempNotification) {
typeLabel.text = notification.type.displayText
- timeLabel.text = "10:00 AM"
- contentLabel.text = "'와쿠와쿠' 님이 소중한 후기를 보내주셨어요! 지금 확인해 볼까요 를레히히"
- backgroundColor = notification.isNew ? .clear : .Background.surface
+ timeLabel.text = notification.time
+ // TODO: notification.type과 notification.name을 사용하여 동적 컨텐츠 생성
+ contentLabel.text = "'\(notification.name)' 님이 소중한 후기를 보내주셨어요! 지금 확인해 볼까요"
+ backgroundColor = notification.isNew ? .Background.surface : .clear // 읽지 않은 알림 강조
}📝 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
| func configure(notification: TempNotification) { | |
| typeLabel.text = notification.type.displayText | |
| timeLabel.text = "10:00 AM" | |
| contentLabel.text = "'와쿠와쿠' 님이 소중한 후기를 보내주셨어요! 지금 확인해 볼까요 를레히히" | |
| backgroundColor = notification.isNew ? .clear : .Background.surface | |
| } | |
| func configure(notification: TempNotification) { | |
| typeLabel.text = notification.type.displayText | |
| timeLabel.text = notification.time | |
| // TODO: notification.type과 notification.name을 사용하여 동적 컨텐츠 생성 | |
| contentLabel.text = "'\(notification.name)' 님이 소중한 후기를 보내주셨어요! 지금 확인해 볼까요" | |
| backgroundColor = notification.isNew ? .Background.surface : .clear // 읽지 않은 알림 강조 | |
| } |
🤖 Prompt for AI Agents
In `@SMASHING/Presentation/Notification/NotificationCell.swift` around lines 86 -
91, In configure(notification: TempNotification) you’re using hardcoded strings
and an inverted background rule; replace the hardcoded "10:00 AM" and sample
content with the passed notification.time (formatted as needed) and
notification.content by assigning to timeLabel.text and contentLabel.text
respectively, and flip the background logic so a new/unread notification
(notification.isNew) uses the highlighted surface color (e.g.,
.Background.surface) while read notifications use .clear; update references in
this method (typeLabel, timeLabel, contentLabel, backgroundColor, and the
TempNotification properties time/content/isNew) accordingly.
Comment on lines
+1
to
+7
| // | ||
| // NotificationViewController.swift | ||
| // SMASHING | ||
| // | ||
| // Created by 이승준 on 1/12/26. | ||
| // | ||
|
|
There was a problem hiding this comment.
헤더 주석의 파일명이 잘못되었습니다.
파일명이 NotificationView.swift인데 주석에는 NotificationViewController.swift로 되어 있습니다.
📝 수정 제안
//
-// NotificationViewController.swift
+// NotificationView.swift
// SMASHING
//
// Created by 이승준 on 1/12/26.
//📝 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
| // | |
| // NotificationViewController.swift | |
| // SMASHING | |
| // | |
| // Created by 이승준 on 1/12/26. | |
| // | |
| // | |
| // NotificationView.swift | |
| // SMASHING | |
| // | |
| // Created by 이승준 on 1/12/26. | |
| // | |
🤖 Prompt for AI Agents
In `@SMASHING/Presentation/Notification/NotificationView.swift` around lines 1 -
7, Update the top-of-file header comment to match the actual filename: replace
"NotificationViewController.swift" with "NotificationView.swift" in the header
comment of NotificationView.swift so the file header accurately reflects the
file name and avoids confusion.
Comment on lines
+43
to
+45
| override func viewDidLoad() { | ||
| setupCollectionView() | ||
| } |
There was a problem hiding this comment.
super.viewDidLoad() 호출이 누락되었습니다.
BaseViewController의 viewDidLoad()에서 view.backgroundColor 설정, setUI(), setLayout() 호출이 이루어집니다. super.viewDidLoad()를 호출하지 않으면 이러한 초기화가 수행되지 않습니다.
🐛 수정 제안
override func viewDidLoad() {
+ super.viewDidLoad()
setupCollectionView()
}📝 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
| override func viewDidLoad() { | |
| setupCollectionView() | |
| } | |
| override func viewDidLoad() { | |
| super.viewDidLoad() | |
| setupCollectionView() | |
| } |
🤖 Prompt for AI Agents
In `@SMASHING/Presentation/Notification/NotificationViewController.swift` around
lines 43 - 45, The override of viewDidLoad() in NotificationViewController is
missing a call to super.viewDidLoad(), which prevents BaseViewController's setup
(view.backgroundColor, setUI(), setLayout()) from running; update the
NotificationViewController.override func viewDidLoad() to call
super.viewDidLoad() at the start of the method before calling
setupCollectionView() so BaseViewController.viewDidLoad() executes and the UI
initialization is applied.
Comment on lines
+59
to
+71
| func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell { | ||
| guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: NotificationCell.reuseIdentifier, for: indexPath) as? NotificationCell else { | ||
| return UICollectionViewCell() | ||
| } | ||
|
|
||
| cell.configure(notification: dummyNotifications[indexPath.item]) | ||
|
|
||
| cell.contentView.snp.makeConstraints { | ||
| $0.width.equalTo(collectionView.frame.width) | ||
| } | ||
|
|
||
| return cell | ||
| } |
There was a problem hiding this comment.
셀 재사용 시 제약조건 충돌이 발생합니다.
cellForItemAt 내에서 snp.makeConstraints를 호출하면 셀이 재사용될 때마다 새로운 제약조건이 추가되어 충돌이 발생합니다. 또한 프로젝트에 타입 안전한 dequeueReusableCell 확장이 있으므로 이를 사용하는 것이 좋습니다.
🐛 수정 제안
func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
- guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: NotificationCell.reuseIdentifier, for: indexPath) as? NotificationCell else {
- return UICollectionViewCell()
- }
-
+ let cell = collectionView.dequeueReusableCell(NotificationCell.self, for: indexPath)
cell.configure(notification: dummyNotifications[indexPath.item])
-
- cell.contentView.snp.makeConstraints {
- $0.width.equalTo(collectionView.frame.width)
- }
-
return cell
}셀 너비는 sizeForItemAt에서 이미 지정되어 있으므로 별도의 제약조건이 필요하지 않습니다.
🤖 Prompt for AI Agents
In `@SMASHING/Presentation/Notification/NotificationViewController.swift` around
lines 59 - 71, In collectionView(_:cellForItemAt:) stop adding constraints to
cell.contentView with snp.makeConstraints (which causes duplicate constraint
conflicts on reuse); remove that block and rely on the item size provided in
collectionView(_:layout:sizeForItemAt:). Use the project's type-safe dequeue
extension instead of dequeueReusableCell(withReuseIdentifier:for:) when
obtaining NotificationCell (still configure with
dummyNotifications[indexPath.item] via NotificationCell.configure), and keep the
guard that casts to NotificationCell but avoid creating new constraints on
reuse.
Comment on lines
+73
to
+75
| func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, sizeForItemAt indexPath: IndexPath) -> CGSize { | ||
| return CGSize(width: collectionView.frame.width, height: 100) // 100은 초기값일 뿐, automaticSize가 이를 덮어씁니다. | ||
| } |
There was a problem hiding this comment.
주석이 실제 동작과 일치하지 않습니다.
주석에서 "automaticSize가 이를 덮어씁니다"라고 되어 있지만, 자동 크기 조정이 설정되어 있지 않습니다. 자동 크기 조정을 사용하려면 UICollectionViewFlowLayout의 estimatedItemSize를 설정해야 합니다. 현재 구현에서는 고정 높이 100이 그대로 사용됩니다.
📝 주석 수정 또는 자동 크기 조정 구현
주석만 수정하는 경우:
- return CGSize(width: collectionView.frame.width, height: 100) // 100은 초기값일 뿐, automaticSize가 이를 덮어씁니다.
+ return CGSize(width: collectionView.frame.width, height: 100)자동 크기 조정이 필요한 경우, NotificationView에서 layout 설정 시:
layout.estimatedItemSize = UICollectionViewFlowLayout.automaticSize📝 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
| func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, sizeForItemAt indexPath: IndexPath) -> CGSize { | |
| return CGSize(width: collectionView.frame.width, height: 100) // 100은 초기값일 뿐, automaticSize가 이를 덮어씁니다. | |
| } | |
| func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, sizeForItemAt indexPath: IndexPath) -> CGSize { | |
| return CGSize(width: collectionView.frame.width, height: 100) | |
| } |
🤖 Prompt for AI Agents
In `@SMASHING/Presentation/Notification/NotificationViewController.swift` around
lines 73 - 75, The comment in collectionView(_:layout:sizeForItemAt:) is
misleading because automatic sizing isn't enabled and the hardcoded
CGSize(width: collectionView.frame.width, height: 100) is used; either update
the comment to reflect this fixed-height behavior or enable automatic cell
sizing by configuring the collection view's flow layout (in NotificationView
where the layout is created) to set layout.estimatedItemSize =
UICollectionViewFlowLayout.automaticSize and then remove or adjust the
collectionView(_:layout:sizeForItemAt:) implementation so it no longer forces a
100pt height (or return UICollectionViewFlowLayout.automaticSize equivalents).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
📌 Related Issue
📷 Screenshots
💬 To Reviewers
Summary by CodeRabbit
릴리스 노트
새로운 기능
스타일
리팩토링
✏️ Tip: You can customize this high-level summary in your review settings.