Skip to content

Conversation

@geeksbaek
Copy link
Collaborator

@geeksbaek geeksbaek commented Dec 31, 2025

Summary

  • CoreData를 iCloud와 동기화하여 여러 기기에서 수집 데이터를 공유할 수 있도록 구현
  • NSPersistentCloudKitContainer에 CloudKit 컨테이너 옵션 설정
  • 원격 변경사항 알림 처리 추가 (NSPersistentStoreRemoteChange)
  • 자동 병합 정책 설정 (automaticallyMergesChangesFromParent, NSMergeByPropertyObjectTrumpMergePolicy)
  • 히스토리 추적 활성화 (NSPersistentHistoryTrackingKey)

주의사항

  • 실제 CloudKit 컨테이너 ID(iCloud.com.leeari95.ACNHWiki)는 프로젝트 설정에 맞게 조정 필요
  • Xcode에서 CloudKit capability 및 Background Modes (Remote notifications) 활성화 필요

Test plan

  • 기기 A에서 아이템 수집 후 기기 B에서 동기화 확인
  • 오프라인 상태에서 수집 후 온라인 복귀 시 동기화 확인
  • 동시 수정 시 병합 정책이 올바르게 적용되는지 확인

Closes #65

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • More robust cloud sync with retry/fallback, background task support, and persistent change history processing.
    • Automatic remote-change notifications and merge handling to keep devices in sync.
  • Bug Fixes / Reliability

    • Safer save operations and store-load readiness checks to reduce data errors.
  • UI/Behavior

    • Minor UI tweaks: selection handling restored to default and adjusted string-check logic affecting character detection.

✏️ Tip: You can customize this high-level summary in your review settings.

- NSPersistentCloudKitContainer에 CloudKit 컨테이너 옵션 설정
- 원격 변경사항 알림 처리 추가 (NSPersistentStoreRemoteChange)
- 자동 병합 정책 설정 (automaticallyMergesChangesFromParent)
- 히스토리 추적 활성화 (NSPersistentHistoryTrackingKey)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Dec 31, 2025

📝 Walkthrough

Walkthrough

Implements a full Core Data + CloudKit stack in CoreDataStorage.swift with CloudKit container selection, persistent store description handling, remote-change notifications, persistent history processing, thread-safe state management, background task APIs, and user-collection merge logic; plus several small UI and string-behavior tweaks elsewhere.

Changes

Cohort / File(s) Summary
Core Data + CloudKit Storage
Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift
New full-featured CoreDataStorage: CloudKit container conditional config, persistent store description with CloudKit options, remote-change observer, persistent history fetching/processing (tokens, purge, merge), thread-safe state (queues, semaphore, flags), store-load wait/retry/fallback, background task API, user-collection fetch/merge APIs, new CoreDataStorageError cases, and Notification.Name.didReceiveRemoteChanges.
String behavior change
Projects/App/Sources/Extension/String+extension.swift
isChosung semantics changed: now breaks on first non-matching char (uses universal check), altering behavior from "any character matches" to "all characters match".
Presentation — Catalog UI
Projects/App/Sources/Presentation/Catalog/ViewControllers/ItemsViewController.swift, .../ViewModels/ItemsReactor.swift, .../Views/CategoryRow.swift, .../Views/ItemSeasonView.swift
Small UI/function tweaks: ignore unused UIAction parameter in two closures; whitespace-only signature change; removed setSelected(_:animated:) override from CategoryRow; replaced optional cast with is type-check in ItemSeasonView—no major control-flow changes except isChosung above.

Sequence Diagram(s)

sequenceDiagram
    %% Styling hints: new/changed interactions highlighted with notes
    participant App as App Init
    participant CDS as CoreDataStorage
    participant PSC as NSPersistentContainer
    participant PS as NSPersistentStore (CloudKit)
    participant CloudKit as CloudKit
    participant Main as Main Thread

    App->>CDS: shared init / setupPersistentContainer()
    activate CDS
    CDS->>PSC: create container & loadPersistentStores()
    PSC->>PS: configure NSPersistentStoreDescription (CloudKit options, history, remote-change)
    PS->>CloudKit: CloudKit sync (background)
    deactivate CDS

    Note over CloudKit,PS: Remote changes synced to local store

    PS->>CDS: NSPersistentStoreRemoteChange notification
    activate CDS
    CDS->>CDS: handleRemoteChange -> enqueue history processing
    CDS->>PS: fetch persistentHistory since last token
    PS-->>CDS: history transactions
    CDS->>CDS: apply merges, advance token, purge old history
    CDS->>Main: post Notification.Name.didReceiveRemoteChanges (main)
    deactivate CDS
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

fix

Poem

🐇 I dug a burrow, snug and neat,
Packed my checks for every feat,
Up to CloudKit, soft and sweet—
When apps depart, my records meet,
Sync and hop, my data's complete.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Most changes are in-scope (CoreDataStorage CloudKit integration), but the PR includes several unrelated modifications: String+extension.swift logic change, ItemsViewController parameter cleanup, ItemsReactor whitespace fix, CategoryRow method removal, and ItemSeasonView type-check refactoring. Remove or separate the unrelated changes in String+extension.swift, ItemsViewController, ItemsReactor, CategoryRow, and ItemSeasonView into independent PRs to maintain focus on the iCloud synchronization feature.
Docstring Coverage ⚠️ Warning Docstring coverage is 59.09% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly indicates the main change is adding iCloud synchronization functionality, which aligns with the extensive Core Data CloudKit integration changes throughout the PR.
Description check ✅ Passed The description follows the template structure, includes issue link, work details, work type, and checklist. However, the Test plan section lacks proper checkbox completion status formatting.
Linked Issues check ✅ Passed The PR successfully implements CloudKit integration with NSPersistentCloudKitContainer, remote change handling, automatic merge policies, and history tracking—directly addressing issue #65's requirement to preserve check records across app uninstall and device changes.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (1)

101-104: Potential duplicate UserCollectionEntity after multi-device sync.

With iCloud sync, if two devices both call getUserCollection before their first sync, each will create a separate UserCollectionEntity. After sync, both entities will exist, and .first returns an arbitrary one (no sort descriptor).

Consider:

  1. Adding a unique constraint or identifier to UserCollectionEntity to prevent duplicates.
  2. Adding deduplication logic that merges or removes duplicates when multiple entities are detected.
  3. Using a stable sort descriptor so behavior is at least deterministic.
🔎 Verification script to check UserCollectionEntity model
#!/bin/bash
# Check if UserCollectionEntity has any unique constraints defined in the Core Data model
fd -e xcdatamodeld -x find {} -name "*.xcdatamodel" -exec cat {}/contents \; 2>/dev/null | grep -A 20 "UserCollectionEntity"
🧹 Nitpick comments (1)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (1)

79-84: Consider debouncing rapid remote change notifications.

When multiple remote changes arrive in quick succession (e.g., initial sync of many items), this will post multiple didReceiveRemoteChanges notifications, potentially causing redundant UI refreshes.

This is a minor optimization concern—the current implementation is functionally correct. If performance issues arise, consider coalescing notifications with a small delay.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 185dea2 and 77434c9.

📒 Files selected for processing (1)
  • Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Check Build
🔇 Additional comments (2)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (2)

35-66: LGTM! CloudKit container setup looks correct.

The configuration follows Apple's recommended pattern for NSPersistentCloudKitContainer:

  • History tracking and remote change notifications are properly enabled before loading stores.
  • automaticallyMergesChangesFromParent and NSMergeByPropertyObjectTrumpMergePolicy are set after store loading, which is the correct sequence.

One consideration: NSMergeByPropertyObjectTrumpMergePolicy means in-memory changes always win over remote changes during conflicts. This aligns with the test plan ("동시 수정 시 병합 정책"), but be aware that whichever device syncs last will have its changes persisted when the same property is modified on multiple devices.


89-91: LGTM!

Clean and idiomatic pattern for defining custom notification names.

beans-go and others added 2 commits January 1, 2026 07:16
- CoreDataStorage CloudKit 통합 개선
- 히스토리 추적 및 중복 병합 로직 강화

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
- performBackgroundTask에서 guard를 if 문으로 변경

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (4)
Animal-Crossing-Wiki/Projects/App/Sources/Extension/String+extension.swift (1)

27-38: Consider using contains for cleaner and more efficient membership check.

The filter + isEmpty pattern allocates an intermediate array unnecessarily. A more idiomatic Swift approach would use contains:

🔎 Suggested refactor
     var isChosung: Bool {
-        var isChosung = false
-        for char in self {
-            if !hangul.filter({ $0.contains(char)}).isEmpty {
-                isChosung = true
-            } else {
-                isChosung = false
-                break
-            }
-        }
-        return isChosung
+        guard !isEmpty else { return false }
+        return allSatisfy { hangul.contains(String($0)) }
     }

This leverages allSatisfy which short-circuits on the first non-matching character and avoids the intermediate array allocation from filter.

Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/ItemSeasonView.swift (1)

75-81: Good refactoring to use is operator for type checking.

The change from an optional cast check to the is operator is more idiomatic Swift and slightly more efficient. The logic correctly prevents duplicate CalendarView instances in the stack.

One minor observation: this refactor appears unrelated to the iCloud sync feature that is the main focus of the PR. Consider grouping unrelated cleanups in separate commits or PRs for easier review tracking.

Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (2)

385-401: Consider returning failure feedback to caller.

When the store isn't loaded and timeout occurs, the method silently returns without executing the block. The caller has no way to know whether their task was executed or skipped.

Consider adding a completion handler or throwing an error to provide feedback:

🔎 Proposed signature with error feedback

Option 1: Throw error

-func performBackgroundTask(_ block: @escaping (NSManagedObjectContext) -> Void) {
+func performBackgroundTask(_ block: @escaping (NSManagedObjectContext) -> Void) throws {
     if !isStoreLoaded {
         logger.warning("performBackgroundTask called before store is loaded. Waiting...")
         guard waitForStoreLoad(timeout: 5.0) else {
             logger.error("Store still not loaded after waiting. Skipping background task.")
-            return
+            throw CoreDataStorageError.storeNotReady
         }
     }
     // ...
 }

Option 2: Completion handler

-func performBackgroundTask(_ block: @escaping (NSManagedObjectContext) -> Void) {
+func performBackgroundTask(_ block: @escaping (NSManagedObjectContext) -> Void, completion: ((Bool) -> Void)? = nil) {
     if !isStoreLoaded {
         logger.warning("performBackgroundTask called before store is loaded. Waiting...")
         guard waitForStoreLoad(timeout: 5.0) else {
             logger.error("Store still not loaded after waiting. Skipping background task.")
+            completion?(false)
             return
         }
     }
     
     persistentContainer.performBackgroundTask { context in
         context.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
         context.transactionAuthor = Bundle.main.bundleIdentifier ?? "ACNHWiki"
         block(context)
+        completion?(true)
     }
 }

42-54: Runtime iCloud state changes are not actively monitored.

isCloudKitEnabled is a computed property that checks FileManager.default.ubiquityIdentityToken != nil, so it reflects the current state on each access. However, CloudKit configuration is only checked during initial persistent store setup (line 119) and error recovery (line 142). If a user signs out of iCloud while the app is running, these checks won't trigger again unless an error occurs or the app restarts.

To handle account changes gracefully, consider observing NSUbiquityIdentityDidChange notifications and updating the data store configuration accordingly.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77434c9 and e85dccc.

📒 Files selected for processing (6)
  • Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift
  • Animal-Crossing-Wiki/Projects/App/Sources/Extension/String+extension.swift
  • Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/ViewControllers/ItemsViewController.swift
  • Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/ViewModels/ItemsReactor.swift
  • Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/CategoryRow.swift
  • Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/ItemSeasonView.swift
💤 Files with no reviewable changes (1)
  • Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/CategoryRow.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/ViewModels/ItemsReactor.swift (2)
Animal-Crossing-Wiki/Projects/App/Sources/Utility/Items.swift (1)
  • updateItem (401-411)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Dashboard/ViewModels/ProgressReactor.swift (1)
  • mutate (33-44)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/ItemSeasonView.swift (2)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/ViewControllers/ItemDetailViewController.swift (1)
  • setUpSaeson (203-213)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/Views/CalendarView.swift (2)
  • configure (59-86)
  • months (10-105)
🪛 SwiftLint (0.57.0)
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift

[Warning] 137-137: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 174-174: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 227-227: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 253-253: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 327-327: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 343-343: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 356-356: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 575-575: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 589-589: Conditional statements should always return on the next line

(conditional_returns_on_newline)


[Warning] 83-83: Computed read-only properties should avoid using the get keyword

(implicit_getter)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Check Build
🔇 Additional comments (15)
Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/ViewModels/ItemsReactor.swift (1)

303-303: LGTM!

Whitespace normalization in the function signature improves code style consistency.

Animal-Crossing-Wiki/Projects/App/Sources/Presentation/Catalog/ViewControllers/ItemsViewController.swift (1)

397-430: LGTM! Good use of Swift conventions.

Both closures correctly use _ to indicate the action parameter is intentionally unused, improving code clarity.

Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift (13)

8-38: LGTM! Error handling is comprehensive.

The new error cases (initializationFailed, persistentStoreDescriptionNotFound, storeNotReady) appropriately cover CloudKit initialization and store loading scenarios.


66-104: LGTM! Thread safety implementation is well-designed.

The use of serial queues (stateQueue, historyProcessingQueue) and computed properties for thread-safe state access is appropriate. The semaphore-based signaling pattern for store load completion is correctly initialized.


119-164: LGTM! CloudKit integration and merge policies are well-configured.

The conditional CloudKit setup based on iCloud availability, remote change notifications, history tracking, and merge policy (NSMergeByPropertyObjectTrumpMergePolicy) are all correctly implemented for conflict resolution in a multi-device sync environment.


192-237: LGTM! Fallback logic is well-implemented.

The local-only fallback correctly removes/destroys the existing store before retrying without CloudKit options. Error logging at each step provides good observability.


241-261: LGTM! Remote change notification handling is thread-safe.

Serializing history processing via historyProcessingQueue prevents concurrent modifications. The early isStoreLoaded check and background context with merge policy are appropriate.


265-316: LGTM! Persistent history processing is correctly implemented.

Token management is thread-safe, changed entity tracking enables granular UI updates, and posting notifications on the main queue is appropriate. Error handling gracefully logs failures without disrupting the app.


318-338: LGTM! History purging is appropriately throttled.

The 24-hour check prevents excessive purging, and 7-day retention balances storage with conflict resolution needs.


342-367: LGTM! Token persistence uses secure coding correctly.

Using NSKeyedArchiver with requiringSecureCoding: true is appropriate for NSPersistentHistoryToken.


372-376: LGTM! Public notification API is well-documented.

Clear documentation of the userInfo structure helps consumers handle remote changes correctly.


403-441: LGTM! User collection fetch APIs are well-designed.

The separation between getUserCollection (fetch-or-create) and fetchExistingUserCollection (fetch-only) provides flexibility. The fetchLimit = 1 optimization and clear documentation about duplicate entities are excellent.


449-497: LGTM! Merge logic handles iCloud duplicates comprehensively.

The strategy of selecting the collection with the most relationships and merging others into it is sound. Duplicate relationship filtering prevents data loss during the merge.


500-564: LGTM! Relationship merging is idempotent and comprehensive.

Using identifier Sets to prevent duplicates and reassigning via back-references follows Core Data best practices. All relationship types are covered.


571-592: LGTM! Context save methods provide appropriate error handling options.

Offering both throwing and non-throwing variants gives callers flexibility based on their error handling needs. The hasChanges check prevents unnecessary I/O.

Comment on lines +111 to +116
guard let description = container.persistentStoreDescriptions.first else {
// description이 없는 경우는 심각한 설정 오류이므로 로그 후 빈 컨테이너 반환
// 이후 isStoreLoaded가 false로 유지되어 안전하게 처리됨
logger.critical("Failed to retrieve a persistent store description. Core Data operations will fail.")
return container
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Search for all accesses to persistentContainer to understand usage patterns
rg "persistentContainer" --type swift -B 2 -A 2

Repository: leeari95/ACNH-wiki

Length of output: 5521


🏁 Script executed:

# Search for isStoreLoaded checks to see if they gate access to persistentContainer
rg "isStoreLoaded" --type swift -B 2 -A 2

Repository: leeari95/ACNH-wiki

Length of output: 7172


🏁 Script executed:

# Look at the full persistentContainer lazy var implementation to understand its initialization
fd "CoreDataStorage.swift" --type f -x head -150 {} | tail -60

Repository: leeari95/ACNH-wiki

Length of output: 2725


Replace the guard block with fatalError to properly signal unrecoverable configuration failure.

Returning an empty container when persistentStoreDescriptions.first is nil creates a safety gap. Storage classes access persistentContainer.viewContext directly without checking isStoreLoaded, so they will attempt operations on an uninitialized container if this guard fails. Since a missing store description is a fatal configuration error that cannot be recovered gracefully, use fatalError instead of returning a useless container.

🤖 Prompt for AI Agents
In
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift
around lines 111–116, the guard that returns an empty container when
container.persistentStoreDescriptions.first is nil should be replaced with a
fatalError to signal an unrecoverable configuration failure; change the guard to
call fatalError with a clear message (including context that
persistentStoreDescriptions.first was nil and Core Data cannot be initialized)
so execution halts instead of returning a partially uninitialized container.

Comment on lines +168 to +188
/// 스토어 로드가 완료될 때까지 대기합니다.
/// - Parameter timeout: 최대 대기 시간 (초)
/// - Returns: 타임아웃 발생 시 false
@discardableResult
func waitForStoreLoad(timeout: TimeInterval = 10.0) -> Bool {
// 이미 로드되었으면 즉시 반환
if isStoreLoaded { return true }

// persistentContainer에 접근하여 lazy 초기화 트리거
_ = persistentContainer

// 로드 완료 대기
let result = storeLoadedSemaphore.wait(timeout: .now() + timeout)
if result == .timedOut {
logger.error("Timed out waiting for store to load.")
return false
}
// 다음 대기자를 위해 다시 signal (semaphore를 바이너리 세마포어처럼 사용)
storeLoadedSemaphore.signal()
return isStoreLoaded
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix semaphore reuse pattern - potential race condition.

The pattern of re-signaling the semaphore (line 186) to allow "the next waiter" is problematic. If multiple threads call waitForStoreLoad concurrently:

  1. Thread A waits, store loads, A receives signal, A re-signals (value = 1)
  2. Thread B waits, immediately passes (value = 0), B re-signals (value = 1)
  3. Semaphore value keeps increasing with each waiter

This defeats the purpose of the semaphore and could cause threads to pass before the store is actually loaded.

🔎 Recommended fix using a condition or simpler pattern

Option 1: Remove re-signaling and use single-use semaphore

 @discardableResult
 func waitForStoreLoad(timeout: TimeInterval = 10.0) -> Bool {
     if isStoreLoaded { return true }
     _ = persistentContainer
     let result = storeLoadedSemaphore.wait(timeout: .now() + timeout)
     if result == .timedOut {
         logger.error("Timed out waiting for store to load.")
         return false
     }
-    // 다음 대기자를 위해 다시 signal (semaphore를 바이너리 세마포어처럼 사용)
-    storeLoadedSemaphore.signal()
     return isStoreLoaded
 }

Option 2: Use DispatchGroup instead

Replace storeLoadedSemaphore with:

-private let storeLoadedSemaphore = DispatchSemaphore(value: 0)
+private let storeLoadGroup = DispatchGroup()

In init():

 private init() {
+    storeLoadGroup.enter()
     loadHistoryToken()
 }

In loadPersistentStores callback:

     logger.info("Persistent store loaded successfully...")
     self.setStoreLoaded(true)
-    self.storeLoadedSemaphore.signal()
+    self.storeLoadGroup.leave()

In waitForStoreLoad:

 func waitForStoreLoad(timeout: TimeInterval = 10.0) -> Bool {
     if isStoreLoaded { return true }
     _ = persistentContainer
-    let result = storeLoadedSemaphore.wait(timeout: .now() + timeout)
-    if result == .timedOut {
+    let result = storeLoadGroup.wait(timeout: .now() + timeout)
+    if result == .timedOut {
         logger.error("Timed out waiting for store to load.")
         return false
     }
-    storeLoadedSemaphore.signal()
     return isStoreLoaded
 }
📝 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
/// 스토어 로드가 완료될 때까지 대기합니다.
/// - Parameter timeout: 최대 대기 시간 (초)
/// - Returns: 타임아웃 발생 시 false
@discardableResult
func waitForStoreLoad(timeout: TimeInterval = 10.0) -> Bool {
// 이미 로드되었으면 즉시 반환
if isStoreLoaded { return true }
// persistentContainer에 접근하여 lazy 초기화 트리거
_ = persistentContainer
// 로드 완료 대기
let result = storeLoadedSemaphore.wait(timeout: .now() + timeout)
if result == .timedOut {
logger.error("Timed out waiting for store to load.")
return false
}
// 다음 대기자를 위해 다시 signal (semaphore를 바이너리 세마포어처럼 사용)
storeLoadedSemaphore.signal()
return isStoreLoaded
}
/// 스토어 로드가 완료될 때까지 대기합니다.
/// - Parameter timeout: 최대 대기 시간 (초)
/// - Returns: 타임아웃 발생 시 false
@discardableResult
func waitForStoreLoad(timeout: TimeInterval = 10.0) -> Bool {
// 이미 로드되었으면 즉시 반환
if isStoreLoaded { return true }
// persistentContainer에 접근하여 lazy 초기화 트리거
_ = persistentContainer
// 로드 완료 대기
let result = storeLoadedSemaphore.wait(timeout: .now() + timeout)
if result == .timedOut {
logger.error("Timed out waiting for store to load.")
return false
}
return isStoreLoaded
}
🧰 Tools
🪛 SwiftLint (0.57.0)

[Warning] 174-174: Conditional statements should always return on the next line

(conditional_returns_on_newline)

🤖 Prompt for AI Agents
In
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift
around lines 168–188, the current pattern re-signals storeLoadedSemaphore after
a waiter returns which allows the semaphore value to grow and creates a race;
replace this with a single-use notification strategy: remove the
storeLoadedSemaphore.signal() call from waitForStoreLoad, ensure the semaphore
is initialized to 0 and only signaled once from the persistent store load
completion path when the store actually finishes loading, or alternatively
replace storeLoadedSemaphore with a DispatchGroup (call group.enter() before
load, group.leave() in the load completion, and group.wait(timeout:) in
waitForStoreLoad); update waitForStoreLoad to simply wait and return
isStoreLoaded without re-signaling.

Comment on lines +487 to +489
if mainCollection.islandReputation == 0 {
mainCollection.islandReputation = duplicateCollection.islandReputation
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Check if islandReputation == 0 is a valid value.

The code treats islandReputation == 0 as "empty" and overwrites it with the duplicate's value. If 0 is a valid reputation value (e.g., starting reputation), this could incorrectly overwrite intentional data.

Consider whether this check should be removed or adjusted:

🔎 Potential fix

If 0 is a valid value, remove this merge logic:

-            if mainCollection.islandReputation == 0 {
-                mainCollection.islandReputation = duplicateCollection.islandReputation
-            }

Or if there's a "not set" sentinel value (e.g., -1), use that instead:

-            if mainCollection.islandReputation == 0 {
+            if mainCollection.islandReputation == -1 {
                 mainCollection.islandReputation = duplicateCollection.islandReputation
             }
📝 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
if mainCollection.islandReputation == 0 {
mainCollection.islandReputation = duplicateCollection.islandReputation
}
🤖 Prompt for AI Agents
In
Animal-Crossing-Wiki/Projects/App/Sources/CoreDataStorage/CoreDataStorage.swift
around lines 487-489, the merge logic treats islandReputation == 0 as "empty"
and overwrites mainCollection with duplicateCollection's value; if 0 is a valid
reputation this is incorrect. Change the check to use a true "unset" sentinel
(e.g., -1) or remove the overwrite entirely: either stop treating 0 as empty and
leave mainCollection.islandReputation unchanged, or adjust the code so it only
overwrites when islandReputation equals the agreed-upon sentinel (and ensure any
existing records use that sentinel when uninitialized).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[피드백] 앱을 제거해도 체크기록을 저장할 수 있는 기능

3 participants