|
| 1 | +# Phase 1: PaywallObserver Mode -- Code Review |
| 2 | + |
| 3 | +**Reviewed:** 2026-03-30 |
| 4 | +**Branch:** `feat/paywall-observer-mode` (10 commits, bc6b4ff..0c4e466) |
| 5 | +**Reviewer:** Code Review Agent |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## Summary |
| 10 | + |
| 11 | +Phase 1 is well-implemented and closely follows the plan and spec. All 9 tasks were completed, both platforms have equivalent functionality, and the code follows existing project patterns. The implementation is clean, well-documented with log statements, and correctly handles the core PaywallObserver flow (native purchase, synchronize, proceed). |
| 12 | + |
| 13 | +--- |
| 14 | + |
| 15 | +## 1. Spec Compliance |
| 16 | + |
| 17 | +**Status: PASS** -- All Phase 1 requirements are met. |
| 18 | + |
| 19 | +| Spec Requirement | Status | Notes | |
| 20 | +|---|---|---| |
| 21 | +| Running mode toggle (Full/Observer) | Done | Both platforms | |
| 22 | +| Persisted in UserDefaults/SharedPrefs | Done | `RunningModeRepository` on both | |
| 23 | +| Re-initializes SDK on toggle | Done | `initPurchasely()` re-called | |
| 24 | +| PurchaseManager (StoreKit 2) | Done | iOS `PurchaseManager.swift` | |
| 25 | +| PurchaseManager (Google Play Billing) | Done | Android `PurchaseManager.kt` | |
| 26 | +| Interceptor branches on mode | Done | Full -> proceed(true), Observer -> native + synchronize + proceed(false) | |
| 27 | +| Promotional offer support (iOS) | Done | `purchaseWithPromoOffer` with `signPromotionalOffer()` | |
| 28 | +| synchronize() after transactions | Done | Both platforms, all code paths | |
| 29 | +| synchronize() on app launch (Observer) | Done | Both platforms | |
| 30 | +| CLAUDE.md updated | Done | New components + gotchas documented | |
| 31 | + |
| 32 | +--- |
| 33 | + |
| 34 | +## 2. Code Quality Assessment |
| 35 | + |
| 36 | +### What was done well |
| 37 | + |
| 38 | +- Clean separation of concerns: `RunningModeRepository` (persistence), `PurchaseManager` (native purchases), and interceptor logic (branching) are properly separated |
| 39 | +- Consistent logging with `[Shaker]` prefix across all new code |
| 40 | +- iOS `synchronize()` correctly uses `success:/failure:` closures (commit 19a8592 fixed this) |
| 41 | +- Android `store_product_id` usage is correct per SDK API (documented in CLAUDE.md gotchas) |
| 42 | +- Proper `@available(iOS 15.0, *)` guards on `PurchaseManager` and all call sites |
| 43 | +- `weak self` used correctly in the SDK start callback on iOS |
| 44 | +- BillingClient reconnection logic on Android is present |
| 45 | + |
| 46 | +### Issues Found |
| 47 | + |
| 48 | +#### Important (should fix) |
| 49 | + |
| 50 | +**I-1: Missing divider between Purchases and SDK Mode sections (Android)** |
| 51 | + |
| 52 | +File: `/Users/kevin/Purchasely/Shaker/android/app/src/main/java/com/purchasely/shaker/ui/screen/settings/SettingsScreen.kt` (line 186-188) |
| 53 | + |
| 54 | +The plan explicitly specified a `Spacer + HorizontalDivider + Spacer` between the "Show Onboarding" button and the "SDK Mode" title, matching the pattern used between every other section. The implementation jumps directly from the button to the section title. |
| 55 | + |
| 56 | +Fix: Insert between lines 186 and 188: |
| 57 | +```kotlin |
| 58 | + Spacer(modifier = Modifier.height(24.dp)) |
| 59 | + HorizontalDivider() |
| 60 | + Spacer(modifier = Modifier.height(24.dp)) |
| 61 | +``` |
| 62 | + |
| 63 | +**I-2: iOS `onChange(of:)` uses `{ newValue in }` single-closure form** |
| 64 | + |
| 65 | +File: `/Users/kevin/Purchasely/Shaker/ios/Shaker/Screens/Settings/SettingsScreen.swift` (lines 69, 156) |
| 66 | + |
| 67 | +The CLAUDE.md gotchas state: "iOS `onChange(of:)` with `{ _, newValue }` requires iOS 17 -- use `{ newValue }` for iOS 16 compat." The implementation uses `{ newValue in }` which is correct for iOS 14-16 but will produce a deprecation warning on iOS 17+. This is fine per the project conventions and is consistent with existing code. **No action needed** -- documenting that this was checked. |
| 68 | + |
| 69 | +**I-3: iOS SettingsScreen double-fires setRunningMode on initial load** |
| 70 | + |
| 71 | +File: `/Users/kevin/Purchasely/Shaker/ios/Shaker/Screens/Settings/SettingsScreen.swift` (line 69-72) |
| 72 | + |
| 73 | +The `onChange(of: viewModel.runningMode)` modifier fires when the Picker selection changes. Since `runningMode` is a `@Published` property and the Picker is bound to it with `$viewModel.runningMode`, the Picker write triggers `onChange`, which calls `viewModel.setRunningMode(newValue)` (writes `runningMode` again) and `appViewModel.initPurchasely()`. The `setRunningMode` call is redundant because the Picker binding already wrote the value. This means: |
| 74 | +1. `RunningModeRepository.shared.runningMode` is set twice (harmless but wasteful) |
| 75 | +2. `initPurchasely()` is called correctly |
| 76 | + |
| 77 | +This is not a bug but a code smell. Consider either removing `setRunningMode` from `onChange` and only calling `appViewModel.initPurchasely()`, or removing the `$viewModel.runningMode` binding and using a manual selection handler. |
| 78 | + |
| 79 | +#### Suggestions (nice to have) |
| 80 | + |
| 81 | +**S-1: Android PurchaseManager only handles SUBS, not INAPP products** |
| 82 | + |
| 83 | +File: `/Users/kevin/Purchasely/Shaker/android/app/src/main/java/com/purchasely/shaker/data/purchase/PurchaseManager.kt` (line 66) |
| 84 | + |
| 85 | +The `purchase()` function hardcodes `BillingClient.ProductType.SUBS`. If the demo ever includes one-time purchases, this would fail silently. The `restore()` function also only queries SUBS. Consider adding an `INAPP` query for completeness, or adding a TODO comment. |
| 86 | + |
| 87 | +**S-2: Android PurchaseManager thread safety** |
| 88 | + |
| 89 | +File: `/Users/kevin/Purchasely/Shaker/android/app/src/main/java/com/purchasely/shaker/data/purchase/PurchaseManager.kt` (line 19) |
| 90 | + |
| 91 | +The `onPurchaseResult` callback field is not synchronized. If `purchase()` is called rapidly twice, the second call overwrites the first callback before `onPurchasesUpdated` fires. For a demo app this is acceptable, but a comment noting the limitation would be helpful. |
| 92 | + |
| 93 | +**S-3: iOS PurchaseManager appAccountToken may silently fail** |
| 94 | + |
| 95 | +File: `/Users/kevin/Purchasely/Shaker/ios/Shaker/Data/PurchaseManager.swift` (lines 23-26) |
| 96 | + |
| 97 | +The `UUID(uuidString:)` will return nil if `anonymousUserId` is not a valid UUID, silently skipping the app account token. This is handled gracefully (purchase proceeds without the token) but a log message when the UUID conversion fails would help debugging. |
| 98 | + |
| 99 | +**S-4: Android `enablePendingPurchases()` without a `PendingPurchaseParams` argument** |
| 100 | + |
| 101 | +File: `/Users/kevin/Purchasely/Shaker/android/app/src/main/java/com/purchasely/shaker/data/purchase/PurchaseManager.kt` (line 23) |
| 102 | + |
| 103 | +Google Play Billing Library 7.x has `enablePendingPurchases(PendingPurchaseParams)`. The parameterless overload still works but is deprecated. Consider using the explicit variant. |
| 104 | + |
| 105 | +--- |
| 106 | + |
| 107 | +## 3. Cross-Platform Consistency |
| 108 | + |
| 109 | +**Status: GOOD** -- Platforms are functionally equivalent with appropriate platform-specific differences. |
| 110 | + |
| 111 | +| Feature | Android | iOS | Consistent? | |
| 112 | +|---|---|---|---| |
| 113 | +| RunningModeRepository | SharedPreferences, Koin singleton | UserDefaults, static shared | Yes (idiomatic per platform) | |
| 114 | +| PurchaseManager | BillingClient, callback-based | StoreKit 2, async/await | Yes (idiomatic per platform) | |
| 115 | +| Interceptor purchase | offerToken from parameters | appleProductId from plan | Yes (different SDK APIs) | |
| 116 | +| Interceptor restore | queryPurchasesAsync + acknowledge | Transaction.currentEntitlements | Yes | |
| 117 | +| Promo offers | N/A (Android uses offer tokens) | signPromotionalOffer + StoreKit 2 | Yes (iOS-only per spec) | |
| 118 | +| synchronize() on launch | Parameterless call | success/failure closures | Yes (different SDK signatures) | |
| 119 | +| Settings UI | SegmentedButtonRow | Picker .segmented | Yes (native patterns) | |
| 120 | +| SDK re-init trigger | ShakerApp.initPurchasely() via cast | appViewModel.initPurchasely() via EnvironmentObject | Yes | |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## 4. Architecture Assessment |
| 125 | + |
| 126 | +- SOLID compliance is good: `PurchaseManager` has a single responsibility (native purchases), `RunningModeRepository` has a single responsibility (persistence), and the interceptor orchestrates them |
| 127 | +- The `ShakerApp` cast (`context.applicationContext as ShakerApp`) on Android is a common pattern for accessing Application-level methods from ViewModels |
| 128 | +- The iOS `@EnvironmentObject private var appViewModel` approach in SettingsScreen is clean and follows SwiftUI conventions |
| 129 | +- No circular dependencies introduced |
| 130 | +- DI registration in Koin is properly ordered |
| 131 | + |
| 132 | +--- |
| 133 | + |
| 134 | +## 5. Verdict |
| 135 | + |
| 136 | +**APPROVED** with one minor fix recommended (I-1: missing divider on Android Settings). |
| 137 | + |
| 138 | +The implementation faithfully delivers every Phase 1 requirement. Code quality is high, cross-platform consistency is strong, and the new components integrate cleanly with the existing architecture. The CLAUDE.md updates with SDK API gotchas (`synchronize()` signatures, `store_product_id` naming) are a valuable addition for future development. |
0 commit comments