MOBILE-187: Remove AppGroup misconfiguration crash#725
Merged
Conversation
Contributor
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses MOBILE-187 / issue #705 by removing crash paths caused by App Group misconfiguration/unavailability and ensuring both the core SDK and logger degrade to local storage instead of trapping. It also adds regression tests covering the previously untested fallback behavior.
Changes:
- Replace
fatalError-based App Group resolution with non-trapping fallbacks (core SDK returns""; logger returnsnil) and add fault logging. - Make logger store URL resolution throw (
FileManager.storeURL) instead of crashing, and propagate viaLoggerDatabaseLoader. - Add regression tests for App Group unavailability and for the storage-transition reporter behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| MindboxTests/DI/AppGroupUnavailableTests.swift | New regression tests covering App Group unavailability fallbacks and transition reporter behavior. |
| MindboxLoggerTests/MBLoggerUtilitiesFetcherTests.swift | New test ensuring logger utilities fetcher never traps. |
| MindboxLoggerTests/FileManagerStoreURLTests.swift | New tests for throwing storeURL + loader fallback to local caches. |
| MindboxLogger/Shared/LoggerRepository/LoggerDatabaseLoader.swift | Update to call throwing FileManager.storeURL with try. |
| MindboxLogger/Shared/Extensions/MBLoggerUtilitiesFetcher.swift | Remove trap; return nil when App Group container can’t be resolved. |
| MindboxLogger/Shared/Extensions/FileManager+Extensions.swift | Replace fatalError with a typed throwing error for App Group container resolution. |
| Mindbox/Utilities/UtilitiesFetcher/MBUtilitiesFetcher.swift | Remove trap; log fault and return "" when App Group is unavailable. |
| Mindbox/PersistenceStorage/MBPersistenceStorage.swift | Add install-state probing helpers + AppGroupStorageTransitionReporter. |
| Mindbox/DI/Injections/InjectReplaceable.swift | Make persistence storage DI fallback to .standard on empty App Group + log on suite init failure. |
| Mindbox/CoreController/CoreController.swift | Invoke transition reporter during initialization. |
| Mindbox.xcodeproj/project.pbxproj | Wire new test files into test targets. |
| Example/Example/NotificationCenter/SwiftDataManager.swift | Avoid crashing the example app when App Group container URL can’t be resolved. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Vailence
approved these changes
Jun 25, 2026
Surfaces the silent re-registration when a broken App Group is later fixed (issue #705 follow-up).
The fallback test overwrote the global buildTestContainer without restoring it, which could leak its minimal builder into later .test-mode suites.
Drop a duplicated/speculative comment (point to AppGroupStorageTransitionReporter), tighten the fetcher docs, and use === for the reporter's store-identity check.
reportIfNeeded() runs before re-registration, so the recovery launch itself doesn't fire; the earliest is the next cold start.
Restore the static MBPersistenceStorage.defaults the fallback test mutates, and add a regression test that a real App Group suite does not read through to .standard (the invariant the transition reporter relies on).
…lback The read-through test verifies generic UserDefaults suite isolation (the runner has no App Group entitlement), not entitlement-specific behavior — rename + reword accordingly. Parameterize the events-store test over [nil, ""] so both defaultDirectoryURL() fallback branches are pinned.
It's an internal forensic record that a fallback->recovery happened, not an integrator-actionable error. It stays persisted to the log store regardless of level (retrievable from exports), so this just stops it spamming the console on every cold start.
Moved after primaryInitialization so the suite marker is already written: it now fires on the recovery launch itself instead of one cold start later. Read-only and gated on the same install marker, so no false positive on clean installs or failed re-registration.
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
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.
Issue
The SDK no longer crashes the host app when the App Group container is unavailable (missing/misconfigured/unprovisioned capability): every fatalError/assertionFailure on that path (core fetcher, DI persistence factory, logger store, Example) now degrades to local storage and surfaces a .fault log instead. Adds a read-only diagnostic that logs when it detects a fallback→App-Group re-registration (install state in both stores), plus regression tests for all the fallback branches. Verified end-to-end on a real iPhone 12 — no crash through the full broken→fixed lifecycle.