feat(KlaviyoCore): add IdentityStore and SDKConfigStore (MAGE-748)#617
Conversation
Introduce two singleton observable stores in KlaviyoCore, readable by any module without importing KlaviyoSwift: IdentityStore (source of truth for ProfileData) and SDKConfigStore (source of truth for the company API key). Each store exposes a read/write protocol split (IdentityReading/IdentityWriting, ConfigReading/ConfigWriting) so consumers depend only on the read interface. Thread safety via a serial DispatchQueue, preserving synchronous reads. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo new Combine-backed store modules are added to ChangesKlaviyoCore Store Modules
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a91b8a. Configure here.
…s reads Wrapping CurrentValueSubject.send in queue.sync delivered subscriber closures synchronously on the store queue, so any subscriber reading current/apiKey in response deadlocked (DispatchQueue.sync is non-reentrant). CurrentValueSubject is already internally synchronized, so the external queue is removed. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
|
Before: After: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/KlaviyoCore/SDKConfigStore.swift`:
- Around line 22-45: Create a reusable generic utility class or extension (such
as ObservableValueStore<T>) that encapsulates the CurrentValueSubject pattern
with its associated getter, publisher, and update method. This utility should
handle the internal synchronization and Combine emission logic currently
duplicated in SDKConfigStore. Replace the apiKeySubject property and its related
methods (apiKey getter, apiKeyPublisher, and updateAPIKey) in SDKConfigStore
with a single composed instance of ObservableValueStore<String?>, following the
same pattern that IdentityStore should also adopt to eliminate duplication and
maintain consistent concurrency behavior across both classes.
In `@Tests/KlaviyoCoreTests/IdentityStoreTests.swift`:
- Around line 78-85: The tuple binding at line 80 uses single-letter variable
names `a` and `b` which violate SwiftLint's identifier_name rules. Rename these
variables to more descriptive names that meet SwiftLint's minimum identifier
length requirements (typically avoiding single-letter names), and update all
references to `a` and `b` in the subsequent XCTAssertEqual assertions to use the
new variable names.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7ed4541f-2aaa-43f1-a7ef-d0f51ae117e4
📒 Files selected for processing (4)
Sources/KlaviyoCore/IdentityStore.swiftSources/KlaviyoCore/SDKConfigStore.swiftTests/KlaviyoCoreTests/IdentityStoreTests.swiftTests/KlaviyoCoreTests/SDKConfigStoreTests.swift
Replace the single apiKey surface on ConfigReading/ConfigWriting with a KlaviyoConfig value type, mirroring the IdentityStore/ProfileData pattern. This lets future config settings (enabled features, data-privacy consent) be added without changing the protocol surface. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/KlaviyoCoreTests/SDKConfigStoreTests.swift`:
- Line 22: The API key string literal "company-123" is repeated in multiple
places within the test (used on lines 22 and 34 in the store.update calls with
KlaviyoConfig). Extract this repeated literal into a shared test constant at the
top of the SDKConfigStoreTests class, then replace both occurrences of the
hardcoded string with references to this constant to improve maintainability and
consistency.
- Around line 52-54: Replace the Task.sleep call that introduces timing
dependency with a deterministic approach by awaiting the stream's initial value
directly after subscription. Remove the sleep statement entirely and instead
await the initial replay from the stream subscription before calling
store.update with the new KlaviyoConfig, then assert on the stream's emitted
value to ensure the update was received. This eliminates the race condition and
makes the test deterministic regardless of CI worker speed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: faf2033b-6c47-4a2e-b69d-4090365d08af
📒 Files selected for processing (2)
Sources/KlaviyoCore/SDKConfigStore.swiftTests/KlaviyoCoreTests/SDKConfigStoreTests.swift
- Extract repeated apiKey literal into a shared test constant - Replace Task.sleep timing dependency in the stream test with a deterministic async iterator to avoid CI flakiness Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Description
Adds two singleton observable stores to
KlaviyoCore—IdentityStore(profile identity) andSDKConfigStore(SDK configuration, starting with the company API key) — so other modules can observe identity/API-key state without importingKlaviyoSwift. Additive only; nothing is wired up or migrated in this PR.Due Diligence
Release/Versioning Considerations
MinorContains changes to the public API.New
publicAPI surface is added toKlaviyoCorebut no existing behavior changes. The stores are not yet read or written by any module — that happens in the follow-up tickets below.Changelog / Code Overview
Part of Phase 1 of the Networking Modularization spec, which moves identity + API-key state out of the
KlaviyoSwiftmonolith intoKlaviyoCoresoKlaviyoFormsandKlaviyoLocationno longer importKlaviyoSwiftjust to observe that state.New files
Sources/KlaviyoCore/IdentityStore.swiftIdentityReading(current,publisher,stream()) /IdentityWriting(update(_:)) protocolsIdentityStoresingleton (.shared) — source of truth forProfileDataSources/KlaviyoCore/SDKConfigStore.swiftConfigReading(apiKey,apiKeyPublisher) /ConfigWriting(updateAPIKey(_:)) protocolsSDKConfigStoresingleton (.shared) — source of truth for the company API keyTests/KlaviyoCoreTests/Design decisions (resolved during implementation, per the ticket)
CompanyObserver,KlaviyoLocationManager) depend on config alone, and gives future config (region/host, feature flags) a home. This overrides the spec's original single bundledIdentityStore.KlaviyoSwift. This documents the one-directional dependency (KlaviyoSwiftwrites, everyone else reads) and lets the implementation change later (e.g. become an actor) without touching consumers. Tests includeMockIdentityReader/MockConfigReaderthat conform to the reading protocols only — a compile-time proof that read consumers get no write access.@_spiwrite enforcement: deferred. Compiler-level enforcement of the write boundary (so same-packageKlaviyoForms/KlaviyoLocationcan't call writes) is deferred to a possible follow-up commit on this branch; the protocol split already documents intent.DispatchQueue, not an actor. Reads are synchronous by design (current/apiKey), matching how consumers will use them; an actor would forceawaiton every read and break that contract. The protocol split keeps an actor migration open for later if async reads become acceptable.How this fits the broader modularization
ProfileData+SDKErrortoKlaviyoCore(merged — blocker for this PR)IdentityStore+SDKConfigStoretoKlaviyoCore— the shared source of truthKlaviyoStateto composeProfileData; wireKlaviyoInternalto push state into these storesKlaviyoForms/KlaviyoLocationobservers to read from these stores, dropping theirKlaviyoSwiftimport for identity/configOnce consumers read from
KlaviyoCore(749 + 750),KlaviyoSwiftinternals (TCA store,KlaviyoStateshape, queue, flush) become private implementation details — unblocking Phase 2 (EventtoKlaviyoCore) and Phase 3 (queue/retry/networking extraction) as module-contained changes.Test Plan
make test-library(run viaxcodebuild teston iPhone 17 Pro simulator). New tests, all passing:IdentityStore.currentis emptyProfileData,SDKConfigStore.apiKeyisnilupdate(_:)/updateAPIKey(_:)reflect synchronously oncurrent/apiKeypublisher/apiKeyPublisher(current value replayed on subscribe, then the update)stream()emits updatesstream()delivers all updates to concurrent consumers with no dropped emissions (100 updates × 2 consumers)MockIdentityReader/MockConfigReadercompile while conforming to the reading protocols onlyRelated Issues/Tickets
Summary by CodeRabbit