feat(AutoPushTracking): Injection + KVO wiring for KlaviyoNotificationDelegate#587
Open
glenn-klaviyo wants to merge 4 commits into
Open
feat(AutoPushTracking): Injection + KVO wiring for KlaviyoNotificationDelegate#587glenn-klaviyo wants to merge 4 commits into
glenn-klaviyo wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a testable UNUserNotificationCenter abstraction, implements gated, idempotent KlaviyoNotificationDelegate injection with delegate-change observation, wires injection into the production environment and SDK initialization, and adds unit tests plus test-environment wiring. ChangesPush Notification Delegate Injection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/KlaviyoSwift/KlaviyoSwiftEnvironment.swift (1)
73-75: ⚡ Quick winExtract the plist key into a constant.
"klaviyo_automatic_push_tracking"is a magic string that is duplicated — it also appears inside the log message inKlaviyoNotificationDelegate.injectIfEnabled(). A shared static constant avoids drift between the two references.♻️ Suggested constant
+private enum InfoPlistKey { + static let automaticPushTracking = "klaviyo_automatic_push_tracking" +} + ... isAutomaticPushTrackingEnabled: { - Bundle.main.object(forInfoDictionaryKey: "klaviyo_automatic_push_tracking") as? Bool == true + Bundle.main.object(forInfoDictionaryKey: InfoPlistKey.automaticPushTracking) as? Bool == true },As per coding guidelines: "Avoid magic strings/numbers, preferring constants, enums and static properties in Swift."
🤖 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 `@Sources/KlaviyoSwift/KlaviyoSwiftEnvironment.swift` around lines 73 - 75, Extract the plist key "klaviyo_automatic_push_tracking" into a single shared constant and replace the literal occurrences: define a static let (e.g., KlaviyoSwiftEnvironment.klaviyoAutomaticPushTrackingKey or a shared KlaviyoKeys struct) and use it inside the isAutomaticPushTrackingEnabled closure in KlaviyoSwiftEnvironment and in KlaviyoNotificationDelegate.injectIfEnabled()’s log message; update both references to use the new constant to avoid the duplicated magic string.Tests/KlaviyoSwiftTests/KlaviyoNotificationDelegateTests.swift (1)
50-53: 💤 Low valueConsider resetting
KlaviyoNotificationDelegate.sharedbetween tests.
sharedis a process-wide singleton whoseexistingDelegate/centerObservationpersist across tests, while onlyklaviyoSwiftEnvironmentis reset ininit(). The current assertions happen to be order-independent, but a future test could become flaky from leaked state. A small teardown that clears the singleton's captured state would harden the suite.🤖 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 `@Tests/KlaviyoSwiftTests/KlaviyoNotificationDelegateTests.swift` around lines 50 - 53, The tests should clear process-wide singleton state for KlaviyoNotificationDelegate between runs: add a teardown (or modify KlaviyoNotificationDelegateTests.init) to reset KlaviyoNotificationDelegate.shared by clearing its captured properties (set shared.existingDelegate = nil and shared.centerObservation = nil) or implement and call a small reset() on KlaviyoNotificationDelegate that does this; keep the existing klaviyoSwiftEnvironment = KlaviyoSwiftEnvironment.test() initialization but ensure the singleton is cleared so tests cannot leak state.
🤖 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 `@Sources/KlaviyoSwift/KlaviyoNotificationDelegate.swift`:
- Around line 82-88: The log message in the iOS 14 branch of the
klaviyoSwiftEnvironment.isAutomaticPushTrackingEnabled() guard exceeds the
110-char SwiftLint limit; update the call that uses Logger.notifications.info so
the message is split into shorter parts (either by breaking into two
Logger.notifications.info calls with a shorter first-line message and a
follow-up line, or by concatenating two shorter string literals) to ensure no
individual string literal exceeds 110 characters while preserving the full
message content and context.
- Around line 25-34: The current observeDelegate(using:) relies on KVO for
UNUserNotificationCenter.delegate and uses MainActor.assumeIsolated, which is
unsupported and can crash; replace the KVO approach in the
UNUserNotificationCenter extension by reading the delegate on the main actor and
invoking the handler when it changes (avoid MainActor.assumeIsolated).
Concretely, implement observeDelegate(using:) to capture the current delegate by
performing Task { `@MainActor` in let current = self.delegate; store it in a small
wrapper object and start a main-actor-safe watcher (e.g., observe app lifecycle
notifications like UIApplication.didBecomeActive or schedule a lightweight
debounce check on the main actor) that re-reads self.delegate on `@MainActor` and
calls the provided handler if the delegate reference differs; ensure the
returned AnyObject is a token that cancels the watcher and that no KVO APIs or
assumeIsolated are used.
---
Nitpick comments:
In `@Sources/KlaviyoSwift/KlaviyoSwiftEnvironment.swift`:
- Around line 73-75: Extract the plist key "klaviyo_automatic_push_tracking"
into a single shared constant and replace the literal occurrences: define a
static let (e.g., KlaviyoSwiftEnvironment.klaviyoAutomaticPushTrackingKey or a
shared KlaviyoKeys struct) and use it inside the isAutomaticPushTrackingEnabled
closure in KlaviyoSwiftEnvironment and in
KlaviyoNotificationDelegate.injectIfEnabled()’s log message; update both
references to use the new constant to avoid the duplicated magic string.
In `@Tests/KlaviyoSwiftTests/KlaviyoNotificationDelegateTests.swift`:
- Around line 50-53: The tests should clear process-wide singleton state for
KlaviyoNotificationDelegate between runs: add a teardown (or modify
KlaviyoNotificationDelegateTests.init) to reset
KlaviyoNotificationDelegate.shared by clearing its captured properties (set
shared.existingDelegate = nil and shared.centerObservation = nil) or implement
and call a small reset() on KlaviyoNotificationDelegate that does this; keep the
existing klaviyoSwiftEnvironment = KlaviyoSwiftEnvironment.test() initialization
but ensure the singleton is cleared so tests cannot leak state.
🪄 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: Repository UI
Review profile: CHILL
Plan: Enterprise
Run ID: e14baead-d7d1-4a52-83ba-0eb903b91665
📒 Files selected for processing (5)
Sources/KlaviyoSwift/Klaviyo.swiftSources/KlaviyoSwift/KlaviyoNotificationDelegate.swiftSources/KlaviyoSwift/KlaviyoSwiftEnvironment.swiftTests/KlaviyoSwiftTests/KlaviyoNotificationDelegateTests.swiftTests/KlaviyoSwiftTests/TestData.swift
glenn-klaviyo
changed the title
glenn-klaviyo
requested review from
ab1470 and
belleklaviyo
and removed request for
a team and
ashiu95
2 tasks
…tialize() - Add UserNotificationCenterProtocol to abstract UNUserNotificationCenter for testability - Add injectIfEnabled() + inject(into:) with KVO observation on the delegate property - Wire injectNotificationDelegate, isAutomaticPushTrackingEnabled, and notificationCenter into KlaviyoSwiftEnvironment so tests control all three without an app-bundle context - Call klaviyoSwiftEnvironment.injectNotificationDelegate() from KlaviyoSDK.initialize(with:) - Add KlaviyoNotificationDelegateTests covering wiring, plist gating, inject behavior, idempotency, and KVO re-injection via MockNotificationCenter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
glenn-klaviyo
force-pushed
the
gb/mage-657-auto-inject-delegate-kvo-plist
branch
from
0b4035b to
3ee11f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Tests/KlaviyoSwiftTests/TestData.swift`:
- Line 234: TestData.swift fails to compile because MockNotificationCenter is
not visible in the test target; move or expose it into shared test support by
creating a common test helper (e.g., define SharedMockNotificationCenter in the
shared test helpers file) and update references in TestData.swift to use that
shared symbol (or unnest/relax access on the existing MockNotificationCenter so
it is public/internal to the test target). Ensure the new
SharedMockNotificationCenter (or the made-visible MockNotificationCenter) lives
in a file included by the test target and retains the same API used by
TestData.swift.
🪄 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: Repository UI
Review profile: CHILL
Plan: Enterprise
Run ID: 97f922e4-2258-426c-95ea-41f18b09c658
📒 Files selected for processing (5)
Sources/KlaviyoSwift/Klaviyo.swiftSources/KlaviyoSwift/KlaviyoNotificationDelegate.swiftSources/KlaviyoSwift/KlaviyoSwiftEnvironment.swiftTests/KlaviyoSwiftTests/KlaviyoNotificationDelegateTests.swiftTests/KlaviyoSwiftTests/TestData.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/KlaviyoSwift/KlaviyoSwiftEnvironment.swift
- Tests/KlaviyoSwiftTests/KlaviyoNotificationDelegateTests.swift
- Sources/KlaviyoSwift/KlaviyoNotificationDelegate.swift
ajaysubra
approved these changes
May 29, 2026
… guard TestData.swift references MockNotificationCenter unconditionally, so on Xcode 15.4 where Swift Testing is unavailable the type was never compiled and the release build failed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…iOS 17 targets
MainActor.assumeIsolated is unavailable below iOS 17; fall back to
Task { @mainactor in } which is safe here because handler is @MainActor-bound
(implicitly Sendable) and the KVO closure does not capture the non-Sendable center.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
Description
Phase 1 ticket #2 of the Automatic Push Open Tracking plan (MAGE-657), scoped to the injection and KVO wiring layer. Delegates the forwarding-cycle guard,
OnceCallback, and fulldidReceive/willPresentimplementations to follow-on PRs.Due Diligence
Release/Versioning Considerations
PatchContains internal changes or backwards-compatible bug fixes.MinorContains changes to the public API.MajorContains breaking changes.Changelog / Code Overview
KlaviyoNotificationDelegate.swiftUserNotificationCenterProtocol— surfacesdelegateandobserveDelegate(using:)so tests can substitute a mock without the app-bundle contextUNUserNotificationCenter.current()requires.UNUserNotificationCentergets a retroactive conformance;MainActor.assumeIsolatedrationale is documented where the KVO closure lives.injectIfEnabled()— reads opt-in flag and center fromKlaviyoSwiftEnvironment, installs proxy when enabled. No plist or center references in the method body itself — both flow through the environment.inject(into:)— private; capturesexistingDelegate, sets proxy as delegate, installs change observer that re-injects if host later overwrites the delegate (SceneDelegate scenario).KlaviyoSwiftEnvironment.swiftinjectNotificationDelegate— callsinjectIfEnabled()on the main actor; wired intoinitialize(with:).isAutomaticPushTrackingEnabled— readsklaviyo_automatic_push_trackingplist key in production; stubbable in tests.notificationCenter— returnsUNUserNotificationCenter.current()in production; stubbable in tests.Klaviyo.swift— one-liner: callsklaviyoSwiftEnvironment.injectNotificationDelegate()frominitialize(with:).KlaviyoNotificationDelegateTests.swift— 6 automated tests viaMockNotificationCenter:initialize(with:)callsinjectNotificationDelegateexactly onceexistingDelegateTest Plan
xcodebuild build -scheme klaviyo-swift-sdk-Package -destination "platform=iOS Simulator,name=iPhone 17 Pro"make test-librarySPMExamplewithklaviyo_automatic_push_tracking = YESin Info.plist:inject(into:)should hit atinitialize(with:)timeUNUserNotificationCenter.delegateassignment inSceneDelegate→ proxy should be re-installed (verify via breakpoint in KVO closure)Related Issues/Tickets
Partially addresses MAGE-657
Blocked by MAGE-649 ✅
Blocks MAGE-659, MAGE-660, MAGE-661
Summary by CodeRabbit
New Features
Tests