feat(example): rebuild JS layer with permission helpers, hooks, and app shell#343
Conversation
a7531e4 to
23b9733
Compare
fe70350 to
91b29af
Compare
23b9733 to
52f894c
Compare
91b29af to
f133ecd
Compare
1460c85 to
9023fb7
Compare
9023fb7 to
9db22f2
Compare
3d2cbb8 to
d5c6e12
Compare
f133ecd to
aaf676d
Compare
b09c314 to
239a0ac
Compare
Introduce PermissionHelper (location + push permission flows wrapping react-native-permissions and @react-native-firebase/messaging) plus four domain hooks — useAnalytics, useForms, useLocation, usePush — each owning the state and SDK calls for one Klaviyo feature area. Hooks provide handlers the UI layer can wire to buttons without touching the SDK directly. - Location permission flow requires separate taps for WhenInUse → Always on iOS (iOS won't prompt twice in one interaction) - usePush re-fetches APNs token on onTokenRefresh so Firebase's FCM token doesn't stomp the APNs token on iOS - Firebase availability is memoized at module scope Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
239a0ac to
69cfdaf
Compare
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
| // Register a background message handler BEFORE AppRegistry.registerComponent. | ||
| // @react-native-firebase/messaging requires the handler to be attached | ||
| // before the app mounts — otherwise iOS may suspend the process before the | ||
| // handler is ready and the background push is dropped. The handler is a | ||
| // no-op here; a real app would forward the payload to | ||
| // Klaviyo.handleBackgroundMessage or similar. | ||
| // The require is guarded so the app still runs when Firebase isn't linked. | ||
| try { | ||
| const messaging = require('@react-native-firebase/messaging').default; | ||
| messaging().setBackgroundMessageHandler(async () => {}); | ||
| } catch { | ||
| // Firebase not available — no-op. | ||
| } | ||
|
|
There was a problem hiding this comment.
I still don't really understand this. Two questions
- What is this, would it confuse our app developers to have it, is the comment clear / accurate?
- Should this be in this PR, or the next one that is more push integration focused I thought
There was a problem hiding this comment.
Good questions. Updated the comment in de2d20a to be much clearer.
What it is: @react-native-firebase/messaging requires you to register a background message handler at module-load time — before AppRegistry.registerComponent. If you don't, it warns on every cold start (No task registered for key ReactNativeFirebaseMessagingHeadlessTask).
For Klaviyo push specifically, this is a no-op. Klaviyo delivers notifications via APNs on iOS and via the FCM token registered with the Klaviyo backend on Android — neither path reaches an FCM background-data-message handler. This stub exists purely to silence the RNFB warning.
Why this PR, not #345: the call is JS-layer code and it's coupled to the @react-native-firebase/messaging dep that this PR adds to package.json. Moving it to #345 would split a single concern (wiring RNFB into the JS entry point) across two PRs. It's safe to keep here because the try/catch guards against Firebase not being linked.
There was a problem hiding this comment.
claude didn't even sign their name
de2d20a to
dc2ac52
Compare
| - DoubleConversion (1.1.6) | ||
| - fast_float (8.0.0) | ||
| - FBLazyVector (0.81.5) | ||
| - Firebase/CoreOnly (12.10.0): |
There was a problem hiding this comment.
Firebase dep chunk starts here. Everything from here through PromisesObjC (~line 60) is pulled in transitively by adding @react-native-firebase/messaging to example/package.json:
Firebase/CoreOnly,Firebase/Messaging,FirebaseCore,FirebaseCoreExtension,FirebaseCoreInternal,FirebaseInstallations,FirebaseMessaging— the actual Firebase iOS SDKGoogleDataTransport,GoogleUtilities/*(7 subspecs) — Google's shared support libs that Firebase depends onnanopb,PromisesObjC— low-level deps (protobuf encoder, Obj-C promises) that GoogleDataTransport and FirebaseInstallations need
None of these are declared directly — they're all autolinked by CocoaPods when pod install sees @react-native-firebase/* in node_modules.
| - React-perflogger (= 0.81.5) | ||
| - React-utils (= 0.81.5) | ||
| - SocketRocket | ||
| - RNFBApp (24.0.0): |
There was a problem hiding this comment.
RN bridging layer for Firebase. RNFBApp and RNFBMessaging (line 2448) are the React Native wrapper pods that expose the Firebase iOS APIs to JS via @react-native-firebase. They sit on top of the native Firebase pods from the top of the file and are added automatically by @react-native-firebase autolinking.
These are the pods affected by the $RNFirebaseAsStaticFramework = true flag in Podfile — that flag tells RNFB to build these wrappers statically so they link cleanly against Firebase's static-only native cores when use_frameworks! is enabled.
| - RNFBApp | ||
| - SocketRocket | ||
| - Yoga | ||
| - RNPermissions (5.4.4): |
There was a problem hiding this comment.
react-native-permissions dep. Just RNPermissions — no Permission-* subspecs appear because permission types are opt-in via the setup.rb invocation in example/ios/Podfile:
node_require('react-native-permissions/scripts/setup.rb')This project only enables the permission macros we actually use (LocationAlways, LocationWhenInUse, Notifications). Every other permission type stays disabled, keeping the binary size minimal.
bc9e74c to
fcb4542
Compare
| [] | ||
| ); | ||
|
|
||
| const renderSection = (sectionKey: string) => { |
There was a problem hiding this comment.
renderSection is redefined on every render because it's a plain function inside App, and since all four domain hooks (useAnalytics, useForms, useLocation, usePush) live here, typing a single character in any text field causes the entire list to re-render.
A possible fix is splitting each case into its own memoized component — e.g. <ProfileSection analytics={analytics} />, <PushSection push={push} /> — so only the section whose state actually changed re-renders. Could be worth improving here if we expect integrators to copy the pattern we use.
There was a problem hiding this comment.
Probably don't expected integrators to copy our view logic but yeah that still sounds annoying/bad.
There was a problem hiding this comment.
Fixed in 265068f. Extracted each switch case into its own React.memo-wrapped component under example/src/sections/ (ProfileSection, EventsSection, FormsSection, GeofencingSection, PushSection). Each takes only the hook result(s) it actually consumes, so a keystroke in one section no longer re-renders the others. renderSection in App.tsx is now a thin switch dispatcher.
There was a problem hiding this comment.
Amend updated (167b8d3) — also includes two additions bundled in the same commit: Styles.ts toggleButtonText base color changed from secondaryText to primary (inactive toggle now reads as clickable, not disabled), and example/ios/Podfile.lock regenerated via pod install with CocoaPods 1.16.2 (Firebase pods confirmed present throughout the chain).
|
|
||
| try { | ||
| const status = await messaging.hasPermission(); | ||
| const AuthorizationStatus = require('@react-native-firebase/messaging') |
There was a problem hiding this comment.
The file already memoizes the Firebase require() call via _firebaseAvailable and getMessagingInstance() to avoid repeated module lookups (noted in the line 7 comment). But AuthorizationStatus is fetched with a separate inline require() inside both requestPushPermission and here in checkPushPermissionStatus, which contradicts that intent. Since AuthorizationStatus is a static property on the default export (not the instance), it could be captured once alongside the existing require() in getMessagingInstance — e.g. returning { instance: messaging(), AuthorizationStatus: messagingModule.AuthorizationStatus } — or just extracted to a module-scope lazy constant on first use.
There was a problem hiding this comment.
Fixed in 265068f. Added a module-scope getAuthorizationStatus() helper that lazily captures AuthorizationStatus once (same pattern as _firebaseAvailable / getMessagingInstance) and guards with try/catch when Firebase isn't linked. Both requestPushPermission and checkPushPermissionStatus now call it instead of inlining require().
There was a problem hiding this comment.
Amend also updated to 167b8d3 — same commit includes the Styles.ts and Podfile.lock additions mentioned above. All four changes (section components, AuthorizationStatus hoist, toggle button color, regenerated Podfile.lock) land in the single amended 2-hooks HEAD. Cascade 4-native → 5-docs → overhaul all clean, no conflicts.
amber-klaviyo
left a comment
There was a problem hiding this comment.
Two suggested improvements in the comments, but overall this looks solid, and seems like a major upgrade!
fcb4542 to
265068f
Compare
4f94732 to
167b8d3
Compare
af931c3 to
6f2487b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ 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 6f2487b. Configure here.
…rage Replace the legacy button-wall demo with an interactive, sectioned example that covers the full Klaviyo SDK public API surface and initializes the SDK from JavaScript instead of native code. - App.tsx: SectionList layout with Profile & Events / Forms / Geofencing / Push sections. App is a pure shell — it owns no hook state, so sibling sections don't re-render when one section's state changes. - Each section component colocates the domain hook it consumes (useAnalytics, useForms, useLocation, usePush). This sidesteps the memoization trap where lifting hooks into App would re-run every hook on any state change and invalidate any React.memo on children. No memo wrappers or useCallback/useMemo gymnastics are needed under this layout. - Profile + Events share a single useAnalytics instance by living in one merged AnalyticsSection (Option C from the review discussion): a typed email in the Profile fields is the same profile events attribute to, and we avoid Context/Provider boilerplate in a demo app. The section renders the profile fields and an inline "Events" sub-header with the event buttons below. - Profile fields: External ID / Email / Phone inputs with individual set buttons, plus a collapsible "Additional Attributes" accordion for first/last name, title, organization and a "Location" accordion for city/country/zip/lat/long, aggregate Set Profile button, Reset Profile - Events: test event + Viewed Product, both with value + uniqueId + custom properties - Push: Firebase-backed permission request, Set Push Token (label reads "APNs Push Token" on iOS and "Firebase Push Token" on Android), Set Badge Count (iOS-only, with number input) - Forms: explicit Register / Unregister - Geofencing: explicit Register / Unregister, Get Current Geofences modal - Deep link handling via Klaviyo.handleUniversalTrackingLink in a Linking useEffect with a proper cleanup - Env loading migrated to react-native-dotenv (.env + .env.example) with a typed @env declaration — no more try/require gymnastics - index.js registers a Firebase background message handler before AppRegistry as required by @react-native-firebase/messaging - Bumps @react-native-firebase/app and /messaging to ^24.0.0 for RN 0.81 bridgeless compatibility; adds react-native-dotenv dev dep - Drops legacy AppViewInterface, KlaviyoReactWrapper, RandomGenerators Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
6f2487b to
85780f7
Compare
#345) # Description Part 3 of 4 in the RN example-app overhaul chain for [MAGE-464](https://linear.app/klaviyo/issue/MAGE-464). Wires up native-level Firebase push integration on both iOS and Android using the JS-first init pattern. Both platforms compile and launch cleanly without Firebase configured (push features are disabled in that mode); full push flow lights up end-to-end when a Firebase config file is present. ## Due Diligence - [x] I have tested this on a simulator/emulator or a physical device, on iOS and Android (if applicable). - [ ] I have added sufficient unit/integration tests of my changes. - [ ] I have adjusted or added new test cases to team test docs, if applicable. - [x] I am confident these changes are implemented with feature parity across iOS and Android (if applicable). ## Release/Versioning Considerations - [x] `Patch` Contains internal changes or backwards-compatible bug fixes. - [ ] `Minor` Contains changes to the public API. - [ ] `Major` Contains **breaking** changes. - [ ] Contains readme or migration guide changes. - [ ] This is planned work for an upcoming release. ## Changelog / Code Overview ### iOS | File | Change | |------|--------| | `AppDelegate.mm` | Call `[FIRApp configure]` on launch (unconditional — stub plist is the zero-config default, documented in #346); preserve UN delegate, deep-link, universal-link, and silent-push handlers; retain `getLaunchOptionsWithURL` helper for RN #32350 cold-start workaround; keep native-init reference block commented | | `Podfile` | `$RNFirebaseAsStaticFramework = true` so RNFirebase links cleanly under `use_frameworks!` | | `Info.plist` | `UIBackgroundModes = [location, remote-notification]`; location usage strings; URL scheme registration | | `*.entitlements` | `aps-environment = development`, wired via `CODE_SIGN_ENTITLEMENTS` | | `project.pbxproj` | Bundle id normalized to `com.klaviyoreactnativesdkexample` (matches Firebase app id and Android `applicationId`); `GoogleService-Info.plist` file reference added so it's bundled when integrators drop it in | | `.github/workflows/ios-build.yml` | Stub `GoogleService-Info.plist` step so CI builds succeed without a real Firebase project (format-valid values so `FirebaseApp.configure()` succeeds at launch; benign backend-registration warning at runtime) | ### Android | File | Change | |------|--------| | `gradle.properties`, `local.properties.template`, `app/build.gradle` | Remove dead `initializeKlaviyoFromNative` / `publicApiKey` / `useNativeFirebase` gradle→BuildConfig plumbing (nothing reads it) | | `app/build.gradle` | Conditionally apply `com.google.gms.google-services` plugin on the presence of `app/google-services.json` — clean build without push configured | | `MainApplication.kt` | Commented reference block for native init; primary path stays in JS | ## Test Plan - [x] iOS: build with stub `GoogleService-Info.plist` (values documented in #346) — app launches, push section shows "Firebase not configured" UI - [x] iOS: build with real plist — `[FIRApp configure]` runs, permission request works (see known sim caveat below) - [x] Android: clean build without `google-services.json` — gms plugin not applied, app launches, push section shows "Firebase not configured" UI - [x] Android: clean build with real `google-services.json` — gms plugin applies, Firebase init succeeds, token populates ### Known iOS simulator caveat Apple has a documented regression on iOS 26.0 / 26.2 simulators where `didRegisterForRemoteNotificationsWithDeviceToken:` never fires (FB19400926, FB19404213; matches [rnfirebase/#8937](invertase/react-native-firebase#8937)). APNs token fetching works correctly on physical devices and iOS 18.x simulators. Setup is correct — Apple's sim is broken. ## Related Issues/Tickets Part of [MAGE-464](https://linear.app/klaviyo/issue/MAGE-464) **Chained PR series:** 1. theme + components (merged in #342) 2. JS layer: permission helpers, hooks, app shell (merged in #343) 3. **This PR** — native platform setup (iOS + Android Firebase push) 4. docs — #346 Also stacked alongside: #347 (CI Play Store publish workflow, branched off this PR). Follow-up: [MAGE-534](https://linear.app/klaviyo/issue/MAGE-534) — convert `AppDelegate.mm` to pure Swift. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
) # Description Part 4 of 4 in the RN example-app overhaul chain for [MAGE-464](https://linear.app/klaviyo/issue/MAGE-464). Adds a live event log for `Klaviyo.registerFormLifecycleHandler` — the new API introduced in RN SDK 2.4.0 — so integrators have a working reference implementation and a built-in protocol inspector for their QA sessions. **Stacked on #345** (`ecm/example-app/4-native`). This PR should be rebased onto `feat/example-app` (or master, whichever is the eventual landing target) once #345 merges; the diff shown here is only the delta on top of that base. ### What changed - **`useForms`** — subscribes to `registerFormLifecycleHandler` on mount (no toggle needed; SDK only emits while forms are registered). Stores events in a ring-buffer (FIFO, capped at 100) so a long QA session doesn't pin an ever-growing array. - **`FormLifecycleEventsModal`** (new) — mirrors the `GeofencesModal` structure. Renders a `FlatList` with per-event timestamps, `formId`/`formName`, event type badge, and `buttonLabel`/`deepLinkUrl` for CTA-click events. Keys are a monotonic id assigned at insertion for stable FlatList reconciler reuse across prepends. Detail fields use `JSON.stringify` so the modal functions as a protocol inspector — `buttonLabel: ""` renders as `""` rather than collapsing (the SDK documents empty-string as a valid value). - **`FormsSection`** — new `ActionButton` showing a live count of captured events that opens the modal. ## Due Diligence - [ ] I have tested this on a simulator/emulator or a physical device, on iOS and Android (if applicable). - [ ] I have added sufficient unit/integration tests of my changes. - [ ] I have adjusted or added new test cases to team test docs, if applicable. - [x] I am confident these changes are implemented with feature parity across iOS and Android (if applicable). _(JS-only change; no platform split.)_ ## Release/Versioning Considerations - [x] `Patch` Contains internal changes or backwards-compatible bug fixes. - [ ] `Minor` Contains changes to the public API. - [ ] `Major` Contains **breaking** changes. - [ ] Contains readme or migration guide changes. - [ ] This is planned work for an upcoming release. > Example app only — no public API changes, no version bump needed. ## Changelog / Code Overview | File | Change | |------|--------| | `example/src/hooks/useForms.ts` | Subscribe to lifecycle handler on mount; maintain ring-buffer event state; return event array + clear fn | | `example/src/components/FormLifecycleEventsModal.tsx` | New modal component — chronological event log with type badge, timestamps, and detail inspector | | `example/src/sections/FormsSection.tsx` | New ActionButton wiring event count → modal | ## Test Plan Here's how it looks <img width="320" height="213" alt="Screenshot 2026-04-21 at 5 06 57 PM" src="https://github.com/user-attachments/assets/102d6396-4fe3-4b68-9499-3432bd6aa926" /> <img width="354" height="765" alt="Screenshot 2026-04-21 at 5 06 46 PM" src="https://github.com/user-attachments/assets/c043f6df-b027-48e5-91c9-8c4b9bb6f2f3" /> - [ ] iOS: trigger a form show event — confirm it appears in the modal with correct `formId`, `formName`, and timestamp - [ ] iOS: trigger a form dismiss — confirm `dismissed` event type badge appears - [ ] iOS: trigger a form CTA click — confirm `buttonLabel` and `deepLinkUrl` are rendered (not collapsed), including when `buttonLabel` is an empty string - [ ] iOS: generate >100 events — confirm older entries are evicted (ring-buffer, not unbounded growth) - [ ] Android: repeat the above — same behavior expected (JS-only change) - [ ] Confirm Clear button resets the list and the count badge on `FormsSection` goes to 0 - [ ] Confirm app cold-starts cleanly with no forms registered — no crash, event count shows 0 ## Related Issues/Tickets Part of [MAGE-464](https://linear.app/klaviyo/issue/MAGE-464) **Chained PR series:** 1. theme + components (merged in #342) 2. JS layer: permission helpers, hooks, app shell (merged in #343) 3. native platform setup — #345 4. **This PR** — lifecycle event subscription + debug modal 5. docs — #346 🤖 Generated with [Claude Code](https://claude.com/claude-code)


Description
Part 2 of 4 in the RN example-app overhaul chain for MAGE-464. Rebuilds the entire JS-layer of the example app:
PermissionHelper— location + push permission flows wrappingreact-native-permissionsand@react-native-firebase/messaginguseAnalytics,useForms,useLocation,usePush, each owning the state and SDK calls for one Klaviyo feature areaApp.tsx) — sectionedSectionListUI that consumes the hooks, wires UI buttons to handlers, loads the API key from env, and fails fast at module load if the key is missingKlaviyoReactWrapper.ts/AppViewInterface.ts/RandomGenerators.tsapproachDue Diligence
Release/Versioning Considerations
PatchContains internal changes or backwards-compatible bug fixes.MinorContains changes to the public API.MajorContains breaking changes.Changelog / Code Overview
example/src/PermissionHelper.ts(new)example/src/hooks/useAnalytics.ts(new)example/src/hooks/useForms.ts(new)example/src/hooks/useLocation.ts(new)example/src/hooks/usePush.ts(new)onTokenRefresh(Android: FCM token; iOS: re-fetch APNs)example/src/App.tsx(rewritten)KLAVIYO_API_KEYexample/src/Styles.tsexample/src/env.d.ts+.env.exampleKey decisions
isApiKeyConfiguredthrough every hook. Matches the Android example app pattern.WhenInUse → Alwaysupgrades are driven by separate user taps, never auto-chained, because iOS silently blocks the upgrade if requested in the same interaction as the initial grant.messaging().requestPermission()triggers APNs registration; we usegetAPNSToken(notgetToken) because Klaviyo iOS expects raw APNs tokens.messaging().app— not just module resolvability — so we correctly detect when[FIRApp configure]was skipped due to missing plist.Test Plan
Related Issues/Tickets
Part of MAGE-464
Chained PR series:
🤖 Generated with Claude Code