Feat/notification#472
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 11
🧹 Nitpick comments (3)
Koin/Presentation/NewHome/Notification/NotificationViewModel.swift (1)
96-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilently swallowed mutation errors can desync UI and store.
All four mutation methods discard failures with
try?. Since the view/table updates optimistically (delete row, mark read, delete-all) without any reload afterward, a failed persistence write leaves the on-screen state diverged from SwiftData with no user feedback; the discrepancy only surfaces on the nextloadNotifications, where deleted/unread rows silently reappear. Consider surfacing a toast (likeloadNotificationsdoes) and/or reloading on failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Koin/Presentation/NewHome/Notification/NotificationViewModel.swift` around lines 96 - 117, The mutation helpers in NotificationViewModel are swallowing persistence failures with try?, which can leave the optimistic UI out of sync with SwiftData. Update deleteNotification(id:), deleteAllNotifications(), markAsRead(id:), and markAllAsRead() to handle thrown errors explicitly instead of discarding them, using the existing toast/error pattern from loadNotifications or by triggering a refresh when a write fails. Ensure the user gets feedback and the view state is reconciled after any failed call to deleteNotificationHistoryUseCase or updateNotificationHistoryUseCase.Koin/Presentation/NewHome/Notification/NotificationViewController.swift (1)
185-188: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the unused
shopIdhere; this route still opens the generic shop list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Koin/Presentation/NewHome/Notification/NotificationViewController.swift` around lines 185 - 188, The navigateToShop(shopId:) method is not using its shopId parameter because this flow still pushes the generic shop list via makeShopViewController(). Remove the unused shopId parameter from the method signature and update any call sites accordingly so the method name and behavior match the actual navigation.Koin/Domain/UseCase/NewHome/FetchNotificationListUseCase.swift (1)
11-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFilename no longer matches the type name.
The file is
FetchNotificationListUseCase.swiftbut now definesFetchNotificationHistoryUseCase/DefaultFetchNotificationHistoryUseCase. Consider renaming the file to match for discoverability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Koin/Domain/UseCase/NewHome/FetchNotificationListUseCase.swift` around lines 11 - 26, The file name no longer matches the types defined inside it. Rename the file to align with FetchNotificationHistoryUseCase and DefaultFetchNotificationHistoryUseCase so it is easier to find and maintain the use case implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Common/Core/NotificationHistoryError.swift`:
- Around line 10-14: NotificationHistoryError currently only conforms to Error,
so error.localizedDescription in NotificationViewModel.loadNotifications() won’t
surface the custom message and the toast falls back to a generic system string.
Update NotificationHistoryError to conform to LocalizedError and make its error
description return the existing message property so the UI shows the intended
text. Keep the change centered on NotificationHistoryError and verify the
calendarDidFail case still exposes "캘린더 오류 발생" through localizedDescription.
In `@Common/NotificationHistory/NotificationHistoryService.swift`:
- Around line 128-145: The cleanup window in
DefaultNotificationHistoryService.deleteExpiredNotifications() is using a 7-day
cutoff, which conflicts with the intended 14-day retention policy. Update the
date calculation in deleteExpiredNotifications() so the expirationDate is based
on 14 days ago, and keep the existing delete/save flow in the MainActor.run
block unchanged.
- Around line 117-125: The deleteAll() method in NotificationHistoryService only
performs container.mainContext.delete(model:) and never persists the change, so
add a save on the same context after the deletion inside the MainActor.run
block, following the pattern used by insert, markAsRead, markAllAsRead, and
delete(messageId:). Ensure the save is awaited and any thrown error is
propagated so the batch deletion is actually committed to the persistent store.
- Around line 34-48: The NotificationHistoryService.insert(record:)
implementation is swallowing persistence failures because `try?` wraps the
`MainActor.run` save path, and unsupported categories are silently ignored in
the `default` branch. Update `insert(record:)` so save errors from
`container.mainContext.save()` are allowed to propagate to callers, and replace
the silent no-op for unsupported categories with an explicit failure path or
validation using the existing `record.category` switch. Keep the fix localized
to `insert(record:)` and the `NotificationRecord` category handling so
`NotificationService.didReceive` can detect dropped or failed inserts.
In `@Common/NotificationHistory/NotificationRecord.swift`:
- Around line 13-21: `NotificationRecord` currently allows duplicate `messageId`
values, which can lead to repeated rows and ambiguous read/delete targeting.
Update the `NotificationRecord` model and any persistence path that creates or
inserts it so `messageId` is enforced as unique, and add deduplication logic
where records are loaded or saved using `messageId` as the key (including the
`NotificationItem.id` mapping) to prevent multiple entries for the same
notification.
- Line 12: Remove Sendable from NotificationRecord since it is a mutable
SwiftData `@Model` class and should not be treated as thread-safe across
Task/MainActor boundaries. Also update NotificationRecord so messageId is
declared as a unique lookup key in the model definition, ensuring markAsRead and
delete operate on a single row and duplicate deliveries cannot create duplicate
records.
In `@Koin/Data/Repository/DefaultNotificationHistoryRepository.swift`:
- Around line 41-43: The public repository API has a naming typo in
DefaultNotificationHistoryRepository: rename makrAllAsRead to markAllAsRead so
it matches the NotificationHistoryRepository protocol and compiles cleanly.
Update the same method name anywhere it is declared or referenced, including the
protocol signature and any conforming implementations, so the async
mark-all-read flow remains consistent.
In `@Koin/Domain/Model/NewHome/NotificationItem.swift`:
- Around line 40-74: The notification mapping in NotificationItem.icon(for:) is
dropping valid AppPath cases because the default branch returns nil, which makes
init?(from:) fail and causes DefaultNotificationHistoryRepository.fetchAll() to
skip persisted records via compactMap. Update the icon mapping so .home, .login,
.club, and .timeTable are either assigned appropriate fallback icons or handled
explicitly, and remove the catch-all default if possible so future AppPath
additions are forced to be mapped at compile time.
In `@Koin/Domain/Repository/NotificationHistoryRepository.swift`:
- Line 15: Rename the misspelled repository API from makrAllAsRead to
markAllAsRead in NotificationHistoryRepository and update every matching
implementation/call site, including DefaultNotificationHistoryRepository and
DefaultUpdateNotificationHistoryUseCase, so the protocol and its uses all share
the corrected method name.
In `@Koin/Presentation/NewHome/Notification/NotificationViewModel.swift`:
- Around line 80-94: The loading reset in
NotificationViewModel.loadNotifications() is happening too early because the
defer runs when the synchronous method returns, not after
fetchNotificationHistoryUseCase.execute() finishes. Move the loading false
cleanup into the Task scope in loadNotifications(), so updateLoading(false) is
sent only after the async fetch completes or fails, keeping updateLoadingState
and refreshControl.endRefreshing() aligned with the real request duration.
In `@NotificationService/NotificationService.swift`:
- Around line 24-37: Persist the notification write before handing off to
contentHandler in NotificationService.didReceive(_:withContentHandler:):
saveNotificationIfAvailable(userInfo:) currently fires an unstructured Task, so
make the save part of the same awaited flow before calling
contentHandler(bestAttemptContent). Update the NotificationService method flow
so the SwiftData insert completes (or is awaited) prior to delivery, and keep
serviceExtensionTimeWillExpire() only as the fallback path.
---
Nitpick comments:
In `@Koin/Domain/UseCase/NewHome/FetchNotificationListUseCase.swift`:
- Around line 11-26: The file name no longer matches the types defined inside
it. Rename the file to align with FetchNotificationHistoryUseCase and
DefaultFetchNotificationHistoryUseCase so it is easier to find and maintain the
use case implementation.
In `@Koin/Presentation/NewHome/Notification/NotificationViewController.swift`:
- Around line 185-188: The navigateToShop(shopId:) method is not using its
shopId parameter because this flow still pushes the generic shop list via
makeShopViewController(). Remove the unused shopId parameter from the method
signature and update any call sites accordingly so the method name and behavior
match the actual navigation.
In `@Koin/Presentation/NewHome/Notification/NotificationViewModel.swift`:
- Around line 96-117: The mutation helpers in NotificationViewModel are
swallowing persistence failures with try?, which can leave the optimistic UI out
of sync with SwiftData. Update deleteNotification(id:),
deleteAllNotifications(), markAsRead(id:), and markAllAsRead() to handle thrown
errors explicitly instead of discarding them, using the existing toast/error
pattern from loadNotifications or by triggering a refresh when a write fails.
Ensure the user gets feedback and the view state is reconciled after any failed
call to deleteNotificationHistoryUseCase or updateNotificationHistoryUseCase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aef731ad-7ef3-47c1-aed1-2baeaf70ca6b
📒 Files selected for processing (21)
Common/Core/AppPath.swiftCommon/Core/NotificationHistoryError.swiftCommon/NotificationHistory/NotificationHistoryService.swiftCommon/NotificationHistory/NotificationRecord.swiftKoin/Data/Repository/DefaultNotificationHistoryRepository.swiftKoin/Domain/Model/NewHome/NotificationIconType.swiftKoin/Domain/Model/NewHome/NotificationItem.swiftKoin/Domain/Repository/NotificationHistoryRepository.swiftKoin/Domain/UseCase/NewHome/DeleteNotificationHistoryUseCase.swiftKoin/Domain/UseCase/NewHome/FetchNotificationListUseCase.swiftKoin/Domain/UseCase/NewHome/UpdateNotificationHistoryUseCase.swiftKoin/Presentation/NewHome/Category/CategoryHostingController.swiftKoin/Presentation/NewHome/Home/HomeHostingController.swiftKoin/Presentation/NewHome/Notification/NotificationViewController.swiftKoin/Presentation/NewHome/Notification/NotificationViewModel.swiftKoin/Presentation/NewHome/Notification/Subviews/NotificationPopUpViewController.swiftKoin/Presentation/NewHome/Notification/Subviews/NotificationTableView/NotificationTableView.swiftKoin/koin.entitlementsNotificationService/NotificationService.entitlementsNotificationService/NotificationService.swiftkoin.xcodeproj/project.pbxproj
💤 Files with no reviewable changes (1)
- Koin/Domain/Model/NewHome/NotificationIconType.swift
| struct NotificationHistoryError: Error { | ||
| let message: String | ||
|
|
||
| static let calendarDidFail = Self.init(message: "캘린더 오류 발생") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files | rg 'Common/Core/NotificationHistoryError\.swift|NotificationViewModel|loadNotifications|NotificationHistoryError'
printf '\n== NotificationHistoryError.swift ==\n'
sed -n '1,120p' Common/Core/NotificationHistoryError.swift
printf '\n== Candidates using NotificationHistoryError or localizedDescription ==\n'
rg -n "NotificationHistoryError|localizedDescription" Common -g '!**/build/**' -g '!**/dist/**'
printf '\n== NotificationViewModel candidate file(s) ==\n'
fd -a 'NotificationViewModel' Common || trueRepository: BCSDLab/KOIN_iOS
Length of output: 1046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' Common/Notification/NotificationViewModel.swiftRepository: BCSDLab/KOIN_iOS
Length of output: 245
🌐 Web query:
Swift Error localizedDescription non LocalizedError generic system message documentation
💡 Result:
In Swift, the localizedDescription property on the Error protocol is not a customization point [1][2]. Because it is defined as an extension on the Error protocol within the Foundation framework rather than a requirement of the protocol itself, it cannot be overridden directly [3][4]. When you access localizedDescription on an error, the default implementation performs a bridge to NSError [3][4]. If your error type does not conform to LocalizedError, this default implementation provides a generic system message (typically indicating that the error domain and code do not have a localized description) [2][3]. To provide a custom, localized message that localizedDescription will return, your error type must conform to the LocalizedError protocol [2][5]. 1. Conforming to LocalizedError: When you conform your type to LocalizedError, you implement the errorDescription property [6][7]. 2. How it works: The Foundation framework's bridging logic checks if an error conforms to LocalizedError [6][8]. If it does, and if errorDescription returns a string, that string is used to populate the NSLocalizedDescriptionKey in the underlying NSError's user info dictionary, which localizedDescription then returns [6][8][7]. 3. Default Behavior: If your type does not conform to LocalizedError, or if the errorDescription property returns nil, localizedDescription falls back to the generic system behavior provided by NSError [2]. Therefore, attempting to override localizedDescription directly in your error type will not work as intended for dynamic dispatch; you must instead implement the errorDescription requirement defined by the LocalizedError protocol [3][5].
Citations:
- 1: https://forums.swift.org/t/confusing-error-localizeddescription-output/5337
- 2: Error.localizedDescription kinda useless, mis-documented swiftlang/swift#68102
- 3: https://stackoverflow.com/questions/42473998/dynamic-runtime-dispatch-in-swift-or-the-strange-way-structs-behave-in-one-man
- 4: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0112-nserror-bridging.md
- 5: https://stackoverflow.com/questions/39176196/how-to-provide-a-localized-description-with-an-error-type-in-swift
- 6: https://github.com/apple/swift/blob/392baefc47d20b56fa685216677160de0f86a134/stdlib/public/Darwin/Foundation/NSError.swift
- 7: https://nshipster.com/swift-foundation-error-protocols/
- 8: https://github.com/apple/swift-corelibs-foundation/blob/6e3a6ac9069f05301e63a93ed365713faf2c5d10/Sources/Foundation/NSError.swift
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "extension\s+Error|extension\s+LocalizedError|errorDescription|localizedDescription" Common -g '!**/build/**' -g '!**/dist/**'Repository: BCSDLab/KOIN_iOS
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== NotificationHistoryError usage ==\n'
rg -n "NotificationHistoryError|localizedDescription" . -g '!**/build/**' -g '!**/dist/**'
printf '\n== NotificationHistoryService.swift ==\n'
sed -n '110,160p' Common/NotificationHistory/NotificationHistoryService.swift
printf '\n== Notification view model files ==\n'
sed -n '1,140p' Koin/Presentation/NewHome/Notification/NotificationViewModel.swift
printf '\n---\n'
sed -n '1,140p' Koin/Presentation/CallVan/CallVanNotification/CallVanNotificationViewModel.swiftRepository: BCSDLab/KOIN_iOS
Length of output: 12862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== NotificationHistoryError usage ==\n'
rg -n "NotificationHistoryError|localizedDescription" . -g '!**/build/**' -g '!**/dist/**'
printf '\n== NotificationHistoryService.swift ==\n'
sed -n '110,160p' Common/NotificationHistory/NotificationHistoryService.swift
printf '\n== Notification view model files ==\n'
sed -n '1,160p' Koin/Presentation/NewHome/Notification/NotificationViewModel.swift
printf '\n---\n'
sed -n '1,160p' Koin/Presentation/CallVan/CallVanNotification/CallVanNotificationViewModel.swiftRepository: BCSDLab/KOIN_iOS
Length of output: 12862
Adopt LocalizedError so the toast shows message.
NotificationViewModel.loadNotifications() surfaces this error with error.localizedDescription, but NotificationHistoryError only conforms to Error, so the toast falls back to a generic system message instead of "캘린더 오류 발생".
🩹 Proposed fix
-struct NotificationHistoryError: Error {
+struct NotificationHistoryError: LocalizedError {
let message: String
static let calendarDidFail = Self.init(message: "캘린더 오류 발생")
+
+ var errorDescription: String? { message }
}📝 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.
| struct NotificationHistoryError: Error { | |
| let message: String | |
| static let calendarDidFail = Self.init(message: "캘린더 오류 발생") | |
| } | |
| struct NotificationHistoryError: LocalizedError { | |
| let message: String | |
| static let calendarDidFail = Self.init(message: "캘린더 오류 발생") | |
| var errorDescription: String? { message } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/Core/NotificationHistoryError.swift` around lines 10 - 14,
NotificationHistoryError currently only conforms to Error, so
error.localizedDescription in NotificationViewModel.loadNotifications() won’t
surface the custom message and the toast falls back to a generic system string.
Update NotificationHistoryError to conform to LocalizedError and make its error
description return the existing message property so the UI shows the intended
text. Keep the change centered on NotificationHistoryError and verify the
calendarDidFail case still exposes "캘린더 오류 발생" through localizedDescription.
| func insert(record: NotificationRecord) async throws { | ||
| guard let container else { | ||
| throw SwiftDataError.loadIssueModelContainer | ||
| } | ||
|
|
||
| switch record.category { | ||
| case .shop, .dining, .keyword, .chat, .callvan, .callvanChat: | ||
| try? await MainActor.run { [weak container] in | ||
| container?.mainContext.insert(record) | ||
| try container?.mainContext.save() | ||
| } | ||
| default: | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
insert swallows persistence errors and silently drops unsupported categories.
try? around MainActor.run discards any error thrown by container?.mainContext.save(), so the async throws signature can never actually surface a real save failure to callers. Combined with the default: break branch, any category outside the whitelist also silently no-ops. Callers (e.g. NotificationService.didReceive persisting incoming pushes) have no way to detect a dropped/failed notification.
🩹 Proposed fix
switch record.category {
case .shop, .dining, .keyword, .chat, .callvan, .callvanChat:
- try? await MainActor.run { [weak container] in
- container?.mainContext.insert(record)
- try container?.mainContext.save()
- }
+ try await MainActor.run {
+ container.mainContext.insert(record)
+ try container.mainContext.save()
+ }
default:
break
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/NotificationHistory/NotificationHistoryService.swift` around lines 34
- 48, The NotificationHistoryService.insert(record:) implementation is
swallowing persistence failures because `try?` wraps the `MainActor.run` save
path, and unsupported categories are silently ignored in the `default` branch.
Update `insert(record:)` so save errors from `container.mainContext.save()` are
allowed to propagate to callers, and replace the silent no-op for unsupported
categories with an explicit failure path or validation using the existing
`record.category` switch. Keep the fix localized to `insert(record:)` and the
`NotificationRecord` category handling so `NotificationService.didReceive` can
detect dropped or failed inserts.
| func deleteAll() async throws { | ||
| guard let container else { | ||
| throw SwiftDataError.loadIssueModelContainer | ||
| } | ||
|
|
||
| try await MainActor.run { | ||
| try container.mainContext.delete(model: NotificationRecord.self) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
deleteAll() never persists the deletion — missing save().
Per SwiftData's documented behavior, batch deletion via delete(model:)/delete(model:where:) is only applied to the persistent store on the next save(). This method never calls save(), unlike every other mutating method in this file (insert, markAsRead, markAllAsRead, delete(messageId:)), so deleteAll() will silently no-op unless a save happens to occur elsewhere afterward.
🩹 Proposed fix
try await MainActor.run {
try container.mainContext.delete(model: NotificationRecord.self)
+ try container.mainContext.save()
}📝 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.
| func deleteAll() async throws { | |
| guard let container else { | |
| throw SwiftDataError.loadIssueModelContainer | |
| } | |
| try await MainActor.run { | |
| try container.mainContext.delete(model: NotificationRecord.self) | |
| } | |
| } | |
| func deleteAll() async throws { | |
| guard let container else { | |
| throw SwiftDataError.loadIssueModelContainer | |
| } | |
| try await MainActor.run { | |
| try container.mainContext.delete(model: NotificationRecord.self) | |
| try container.mainContext.save() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/NotificationHistory/NotificationHistoryService.swift` around lines 117
- 125, The deleteAll() method in NotificationHistoryService only performs
container.mainContext.delete(model:) and never persists the change, so add a
save on the same context after the deletion inside the MainActor.run block,
following the pattern used by insert, markAsRead, markAllAsRead, and
delete(messageId:). Ensure the save is awaited and any thrown error is
propagated so the batch deletion is actually committed to the persistent store.
| extension DefaultNotificationHistoryService { | ||
| private func deleteExpiredNotifications() async throws { | ||
| guard let container else { | ||
| throw SwiftDataError.loadIssueModelContainer | ||
| } | ||
|
|
||
| guard let expirationDate = Calendar.current.date(byAdding: .day, value: -7, to: Date()) else { | ||
| throw NotificationHistoryError.calendarDidFail | ||
| } | ||
|
|
||
| try await MainActor.run { | ||
| try container.mainContext.delete(model: NotificationRecord.self, where: #Predicate { notification in | ||
| notification.createdAt < expirationDate | ||
| }) | ||
| try container.mainContext.save() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retention window is 7 days, but PR intent states 14 days.
The commit summary describes "deleting notification records older than 14 days when reading them," but this cleanup uses value: -7. If 14 days is the intended retention policy, this needs correcting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/NotificationHistory/NotificationHistoryService.swift` around lines 128
- 145, The cleanup window in
DefaultNotificationHistoryService.deleteExpiredNotifications() is using a 7-day
cutoff, which conflicts with the intended 14-day retention policy. Update the
date calculation in deleteExpiredNotifications() so the expirationDate is based
on 14 days ago, and keep the existing delete/save flow in the MainActor.run
block unchanged.
| import Foundation | ||
|
|
||
| @Model | ||
| final class NotificationRecord: Sendable { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files 'Common/NotificationHistory/*' 'Common/*Notification*' '*/NotificationService.swift' '*/NotificationHistoryService.swift'
printf '\n== Outline: NotificationRecord.swift ==\n'
ast-grep outline Common/NotificationHistory/NotificationRecord.swift --view expanded || true
printf '\n== NotificationRecord.swift ==\n'
cat -n Common/NotificationHistory/NotificationRecord.swift
printf '\n== NotificationService.swift matches ==\n'
rg -n "NotificationRecord|Task\s*\{|MainActor\.run|insert\(" Common/NotificationHistory NotificationService.swift Common -g '*.swift' || true
printf '\n== NotificationHistoryService.swift matches ==\n'
rg -n "NotificationRecord|Task\s*\{|MainActor\.run|insert\(" Common/NotificationHistory NotificationHistoryService.swift Common -g '*.swift' || trueRepository: BCSDLab/KOIN_iOS
Length of output: 11592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Relevant Swift files ==\n'
fd -a 'Notification(Service|HistoryService|Record)\.swift$' . || true
printf '\n== Search for Sendable / `@Model` / unique ==\n'
rg -n "`@Model`|Sendable|`@Attribute`\\(\\.unique\\)|messageId|PersistentIdentifier" Common -g '*.swift' || trueRepository: BCSDLab/KOIN_iOS
Length of output: 1546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== NotificationHistoryService.swift (1-220) ==\n'
cat -n Common/NotificationHistory/NotificationHistoryService.swift
printf '\n== NotificationService.swift (1-220) ==\n'
cat -n NotificationService/NotificationService.swiftRepository: BCSDLab/KOIN_iOS
Length of output: 10492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path('Common/NotificationHistory/NotificationHistoryService.swift'),
Path('NotificationService/NotificationService.swift'),
]:
print(f"\n== {path} ==")
for i, line in enumerate(path.read_text().splitlines(), 1):
if 1 <= i <= 220:
print(f"{i:4d}: {line}")
PYRepository: BCSDLab/KOIN_iOS
Length of output: 10278
Drop Sendable from NotificationRecord, and make messageId unique.
NotificationRecordis a mutable SwiftData@Modelclass, soSendableonly masks actor-isolation diagnostics when the instance crosses intoTask {}/MainActor.run.messageIdis the lookup key formarkAsReadanddelete; without a uniqueness constraint, duplicate deliveries can create duplicate rows and inconsistent history.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/NotificationHistory/NotificationRecord.swift` at line 12, Remove
Sendable from NotificationRecord since it is a mutable SwiftData `@Model` class
and should not be treated as thread-safe across Task/MainActor boundaries. Also
update NotificationRecord so messageId is declared as a unique lookup key in the
model definition, ensuring markAsRead and delete operate on a single row and
duplicate deliveries cannot create duplicate records.
| func makrAllAsRead() async throws { | ||
| try await service.markAllAsRead() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo: makrAllAsRead → markAllAsRead.
Naming typo in the public repository API. Since this must match the NotificationHistoryRepository protocol signature to compile, the same typo likely exists there too — worth fixing in both places while it's still low-diff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Koin/Data/Repository/DefaultNotificationHistoryRepository.swift` around lines
41 - 43, The public repository API has a naming typo in
DefaultNotificationHistoryRepository: rename makrAllAsRead to markAllAsRead so
it matches the NotificationHistoryRepository protocol and compiles cleanly.
Update the same method name anywhere it is declared or referenced, including the
protocol signature and any conforming implementations, so the async
mark-all-read flow remains consistent.
|
|
||
| extension NotificationItem { | ||
|
|
||
| init?(from record: NotificationRecord) { | ||
| guard let icon = NotificationItem.icon(for: record.category) else { | ||
| return nil | ||
| } | ||
| self.id = record.messageId | ||
| self.isRead = record.isRead | ||
| self.icon = icon | ||
| self.appPath = record.category | ||
| self.uri = record.schemeUri | ||
| self.title = record.title | ||
| self.content = record.body | ||
| self.dateText = NotificationItem.dateText(for: record.createdAt) | ||
| } | ||
|
|
||
| static func icon(for appPath: AppPath) -> ImageAsset? { | ||
| switch appPath { | ||
| case .shop: | ||
| return ImageAsset.notificationShop | ||
| case .dining: | ||
| return ImageAsset.notificationDining | ||
| case .keyword: | ||
| return ImageAsset.notificationLostItem | ||
| case .chat: | ||
| return ImageAsset.notificationChat | ||
| case .callvan: | ||
| return ImageAsset.notificationCallVan | ||
| case .callvanChat: | ||
| return ImageAsset.notificationChat | ||
| default: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Notifications for .home, .login, .club, .timeTable categories are silently dropped from history.
icon(for:)'s default: return nil causes init?(from:) to return nil, and DefaultNotificationHistoryRepository.fetchAll() uses compactMap, so any persisted NotificationRecord with one of the unhandled AppPath cases (.home, .login, .club, .timeTable) is stored but never shown to the user, with no error surfaced. Since these are valid AppPath cases that NotificationService.saveNotificationIfAvailable will happily persist, this isn't just a future-proofing concern.
Consider either providing a fallback icon for the remaining cases, or making the switch exhaustive (dropping default) so adding a new AppPath case forces this mapping to be revisited at compile time.
♻️ Example fix
static func icon(for appPath: AppPath) -> ImageAsset? {
switch appPath {
case .shop:
return ImageAsset.notificationShop
case .dining:
return ImageAsset.notificationDining
case .keyword:
return ImageAsset.notificationLostItem
case .chat:
return ImageAsset.notificationChat
case .callvan:
return ImageAsset.notificationCallVan
case .callvanChat:
return ImageAsset.notificationChat
- default:
- return nil
+ case .home, .login, .club, .timeTable:
+ return ImageAsset.notificationDefault // or an appropriate fallback
}
}📝 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.
| extension NotificationItem { | |
| init?(from record: NotificationRecord) { | |
| guard let icon = NotificationItem.icon(for: record.category) else { | |
| return nil | |
| } | |
| self.id = record.messageId | |
| self.isRead = record.isRead | |
| self.icon = icon | |
| self.appPath = record.category | |
| self.uri = record.schemeUri | |
| self.title = record.title | |
| self.content = record.body | |
| self.dateText = NotificationItem.dateText(for: record.createdAt) | |
| } | |
| static func icon(for appPath: AppPath) -> ImageAsset? { | |
| switch appPath { | |
| case .shop: | |
| return ImageAsset.notificationShop | |
| case .dining: | |
| return ImageAsset.notificationDining | |
| case .keyword: | |
| return ImageAsset.notificationLostItem | |
| case .chat: | |
| return ImageAsset.notificationChat | |
| case .callvan: | |
| return ImageAsset.notificationCallVan | |
| case .callvanChat: | |
| return ImageAsset.notificationChat | |
| default: | |
| return nil | |
| } | |
| } | |
| extension NotificationItem { | |
| init?(from record: NotificationRecord) { | |
| guard let icon = NotificationItem.icon(for: record.category) else { | |
| return nil | |
| } | |
| self.id = record.messageId | |
| self.isRead = record.isRead | |
| self.icon = icon | |
| self.appPath = record.category | |
| self.uri = record.schemeUri | |
| self.title = record.title | |
| self.content = record.body | |
| self.dateText = NotificationItem.dateText(for: record.createdAt) | |
| } | |
| static func icon(for appPath: AppPath) -> ImageAsset? { | |
| switch appPath { | |
| case .shop: | |
| return ImageAsset.notificationShop | |
| case .dining: | |
| return ImageAsset.notificationDining | |
| case .keyword: | |
| return ImageAsset.notificationLostItem | |
| case .chat: | |
| return ImageAsset.notificationChat | |
| case .callvan: | |
| return ImageAsset.notificationCallVan | |
| case .callvanChat: | |
| return ImageAsset.notificationChat | |
| case .home, .login, .club, .timeTable: | |
| return ImageAsset.notificationDefault // or an appropriate fallback | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Koin/Domain/Model/NewHome/NotificationItem.swift` around lines 40 - 74, The
notification mapping in NotificationItem.icon(for:) is dropping valid AppPath
cases because the default branch returns nil, which makes init?(from:) fail and
causes DefaultNotificationHistoryRepository.fetchAll() to skip persisted records
via compactMap. Update the icon mapping so .home, .login, .club, and .timeTable
are either assigned appropriate fallback icons or handled explicitly, and remove
the catch-all default if possible so future AppPath additions are forced to be
mapped at compile time.
| func deleteAll() async throws | ||
| func delete(id: String) async throws | ||
| func markAsRead(id: String) async throws | ||
| func makrAllAsRead() async throws |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Typo: makrAllAsRead → markAllAsRead.
This new protocol method name is misspelled. It's consistently used in DefaultNotificationHistoryRepository and DefaultUpdateNotificationHistoryUseCase, so fixing it now (before this API is more widely adopted) is cheap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Koin/Domain/Repository/NotificationHistoryRepository.swift` at line 15,
Rename the misspelled repository API from makrAllAsRead to markAllAsRead in
NotificationHistoryRepository and update every matching implementation/call
site, including DefaultNotificationHistoryRepository and
DefaultUpdateNotificationHistoryUseCase, so the protocol and its uses all share
the corrected method name.
| private func loadNotifications() { | ||
| outputSubject.send(.updateLoading(true)) | ||
| fetchNotificationListUseCase.execute().sink( | ||
| receiveCompletion: { [weak self] completion in | ||
| self?.outputSubject.send(.updateLoading(false)) | ||
| if case .failure(let error) = completion { | ||
| self?.outputSubject.send(.showToast(error.message)) | ||
| } | ||
| }, | ||
| receiveValue: { [weak self] notifications in | ||
| guard let self else { return } | ||
| self.outputSubject.send(.updateNotifications(notifications)) | ||
| self.outputSubject.send(.updateLoading(false)) | ||
| } | ||
| ) | ||
| .store(in: &subscriptions) | ||
| defer { | ||
| outputSubject.send(.updateLoading(false)) | ||
| } | ||
|
|
||
| Task { | ||
| do { | ||
| let notifications = try await fetchNotificationHistoryUseCase.execute() | ||
| outputSubject.send(.updateNotifications(notifications)) | ||
| } catch { | ||
| outputSubject.send(.showToast(error.localizedDescription)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
defer fires before the async fetch completes.
loadNotifications() is synchronous, so the defer runs when the function returns — which is immediately after the Task is scheduled, not after fetchNotificationHistoryUseCase.execute() resolves. As a result updateLoading(false) is emitted right away, the loading indicator never reflects the real fetch duration, and (via updateLoadingState) refreshControl.endRefreshing() ends before data arrives. Move the defer inside the Task.
🐛 Proposed fix
private func loadNotifications() {
outputSubject.send(.updateLoading(true))
- defer {
- outputSubject.send(.updateLoading(false))
- }
-
Task {
+ defer {
+ outputSubject.send(.updateLoading(false))
+ }
do {
let notifications = try await fetchNotificationHistoryUseCase.execute()
outputSubject.send(.updateNotifications(notifications))
} catch {
outputSubject.send(.showToast(error.localizedDescription))
}
}
}📝 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.
| private func loadNotifications() { | |
| outputSubject.send(.updateLoading(true)) | |
| fetchNotificationListUseCase.execute().sink( | |
| receiveCompletion: { [weak self] completion in | |
| self?.outputSubject.send(.updateLoading(false)) | |
| if case .failure(let error) = completion { | |
| self?.outputSubject.send(.showToast(error.message)) | |
| } | |
| }, | |
| receiveValue: { [weak self] notifications in | |
| guard let self else { return } | |
| self.outputSubject.send(.updateNotifications(notifications)) | |
| self.outputSubject.send(.updateLoading(false)) | |
| } | |
| ) | |
| .store(in: &subscriptions) | |
| defer { | |
| outputSubject.send(.updateLoading(false)) | |
| } | |
| Task { | |
| do { | |
| let notifications = try await fetchNotificationHistoryUseCase.execute() | |
| outputSubject.send(.updateNotifications(notifications)) | |
| } catch { | |
| outputSubject.send(.showToast(error.localizedDescription)) | |
| } | |
| } | |
| } | |
| private func loadNotifications() { | |
| outputSubject.send(.updateLoading(true)) | |
| Task { | |
| defer { | |
| outputSubject.send(.updateLoading(false)) | |
| } | |
| do { | |
| let notifications = try await fetchNotificationHistoryUseCase.execute() | |
| outputSubject.send(.updateNotifications(notifications)) | |
| } catch { | |
| outputSubject.send(.showToast(error.localizedDescription)) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Koin/Presentation/NewHome/Notification/NotificationViewModel.swift` around
lines 80 - 94, The loading reset in NotificationViewModel.loadNotifications() is
happening too early because the defer runs when the synchronous method returns,
not after fetchNotificationHistoryUseCase.execute() finishes. Move the loading
false cleanup into the Task scope in loadNotifications(), so
updateLoading(false) is sent only after the async fetch completes or fails,
keeping updateLoadingState and refreshControl.endRefreshing() aligned with the
real request duration.
|
|
||
| // MARK: - SwiftData에 Notification 데이터를 기록한다. | ||
| saveNotificationIfAvailable(userInfo: bestAttemptContent.userInfo) | ||
|
|
||
| // MARK: - 푸시알림이 이미지를 포함하는 경우 처리 | ||
| if let fcmOptionsUserInfo = bestAttemptContent.userInfo["fcm_options"] as? [String: Any], | ||
| let imageURLString = fcmOptionsUserInfo["image"] as? String, | ||
| let imageURL = URL(string: imageURLString) { | ||
|
|
||
| // 이미지 다운로드 | ||
| if let imageData = try? Data(contentsOf: imageURL) { | ||
| // UNNotificationAttachment 설정 | ||
| if let attachment = UNNotificationAttachment.saveImageToDisk(identifier: "certificationImage.jpg", data: imageData, options: nil) { | ||
| bestAttemptContent.attachments = [attachment] | ||
| contentHandler(bestAttemptContent) | ||
| } else { | ||
| contentHandler(bestAttemptContent) | ||
| } | ||
| } else { | ||
| contentHandler(bestAttemptContent) | ||
| } | ||
| } else { | ||
| contentHandler(bestAttemptContent) | ||
| let imageURL = URL(string: imageURLString), | ||
| let imageData = try? Data(contentsOf: imageURL), | ||
| let attachment = UNNotificationAttachment.saveImageToDisk(identifier: "certificationImage.jpg", data: imageData, options: nil) { | ||
| bestAttemptContent.attachments = [attachment] | ||
| } | ||
|
|
||
| contentHandler(bestAttemptContent) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant ranges.
git ls-files | rg '^NotificationService/NotificationService\.swift$'
wc -l NotificationService/NotificationService.swift
sed -n '1,220p' NotificationService/NotificationService.swiftRepository: BCSDLab/KOIN_iOS
Length of output: 4256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the persistence implementation and any extension-time handling.
rg -n "DefaultNotificationHistoryService|insert\\(record:|serviceExtensionTimeWillExpire|Task \\{" NotificationService -g '!**/xcuserdata/**'
# Inspect the persistence service files if present.
for f in $(git ls-files | rg 'DefaultNotificationHistoryService|NotificationHistoryService|NotificationRecord'); do
echo "===== $f ====="
wc -l "$f"
sed -n '1,240p' "$f"
doneRepository: BCSDLab/KOIN_iOS
Length of output: 5912
Persist the notification write before contentHandler. saveNotificationIfAvailable(userInfo:) launches an unstructured Task, and didReceive calls contentHandler(bestAttemptContent) immediately after. If the extension is suspended or terminated after that handoff, the SwiftData insert can be cancelled and the notification history entry is lost. Run the save in the same awaited flow, and keep serviceExtensionTimeWillExpire() as a best-attempt fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NotificationService/NotificationService.swift` around lines 24 - 37, Persist
the notification write before handing off to contentHandler in
NotificationService.didReceive(_:withContentHandler:):
saveNotificationIfAvailable(userInfo:) currently fires an unstructured Task, so
make the save part of the same awaited flow before calling
contentHandler(bestAttemptContent). Update the NotificationService method flow
so the SwiftData insert completes (or is awaited) prior to delivery, and keep
serviceExtensionTimeWillExpire() only as the fallback path.
1 participant
#️⃣연관된 이슈
📝작업 내용
홈화면 리디자인 알림화면 기능 구현 작업입니다.
AppGroup 설정
Koin과 NotificationService를 하나의 AppGroup으로 설정했습니다.
SwiftData 저장소 만들기
SwiftData 를 사용했습니다.
내부적으로 DataContainer를 사용하는 NotificationHistoryService를 만들었습니다.
NotificationService에서 푸시알림을 SwiftData에 저장
NotificationService의 didReceive에서 푸시알림을 저장합니다.
알림화면 기능 연결
알림화면에서 기능을 연결했습니다.
화면 전환 등
알림 선택시 화면 전환 등을 마저 구현했습니다.
스크린샷 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
New Features
Bug Fixes