fix(iaf): use bridge-provided context instead of persisted state#539
fix(iaf): use bridge-provided context instead of persisted state#539
Conversation
Adds lifecycle event callbacks for in-app forms to enable tracking form interactions in third-party analytics platforms (Amplitude, Segment, etc.). - `registerFormLifecycleHandler(_:)` - Register callback for form events - `unregisterFormLifecycleHandler()` - Remove callback - `isFormLifecycleHandlerRegistered` - Check registration status - `formShown` - Fires when form is presented - `formDismissed` - Fires when form is dismissed (any reason) - `formCTAClicked` - Fires when user taps a CTA button - Events fire on main thread via @mainactor - Handler persists across multiple `registerForInAppForms()` calls - Follows existing `registerDeepLinkHandler()` pattern - Optional feature - forms work without handler Also fixes a bug where forms with empty deep link URLs would fail silently when CTA buttons were clicked. Now handles empty URLs gracefully and still fires lifecycle events. \`\`\`swift KlaviyoSDK().registerFormLifecycleHandler { event in switch event { case .formShown: Analytics.track("Klaviyo Form Shown") case .formDismissed: Analytics.track("Klaviyo Form Dismissed") case .formCTAClicked: Analytics.track("Klaviyo Form CTA Clicked") } } \`\`\` Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes 3 critical issues identified in code review: 1. **Security: Remove real API key from example** - Reverted "Xr5bFG" to placeholder "YOUR_PUBLIC_API_KEY" - Prevents accidental credential exposure in public repository 2. **Reliability: Fire formDismissed for all dismissal paths** - Added lifecycle event invocation to destroyWebView() - Ensures timeout-based and programmatic dismissals fire events - Matches documentation promise: "fires for all dismissal types" - Only fires if form is actually visible (presentingViewController != nil) 3. **Robustness: Handle missing/null deep link fields** - Changed from decode() to decodeIfPresent() for ios field - Gracefully handles null, missing, or empty URL values - Prevents decode failures from dropping lifecycle events - Maintains backward compatibility with valid URLs These changes improve security, ensure complete event coverage, and make the bridge more resilient to varied JavaScript payloads.
Added flag to track whether formDismissed has been fired for current form session. This prevents duplicate events when both dismissForm() and destroyWebView() are called during shutdown. Flag is reset when a new form is presented. Addresses: #515 (comment) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed unnecessary isFormLifecycleHandlerRegistered API to simplify the public interface. Registration can't fail, and users don't need to check if a handler is registered. This aligns with other SDK features that don't expose similar "is registered" properties. Changes: - Removed public isFormLifecycleHandlerRegistered property - Removed internal hasFormLifecycleHandler property - Updated tests to verify behavior instead of checking property - Removed example usage from AppDelegate - Updated README documentation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove `package` access modifier from lifecycle handler methods (internal is sufficient, both callers are in KlaviyoForms) - Move Form Lifecycle Handler MARK section below Properties & Initializer to reflect its secondary importance - Add privacy: .public to unknown message handler warning log - Remove raw JSON log on decode failure (potential PII in payload) - Strip verbose print statements and Amplitude/Mixpanel references from example app - Remove redundant Thread.isMainThread assertion (@mainactor already guarantees main thread) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Passes the ID of the active form alongside the lifecycle event so callers can attribute analytics events to a specific form. Changes: - IAFNativeBridgeEvent: decode formId from formWillAppear payload - IAFLifecycleEvent: add associated value to .present case - IAFPresentationManager: track currentFormId; pass it to handler - KlaviyoSDK+Forms: update public registerFormLifecycleHandler signature to (FormLifecycleEvent, String?) -> Void - Update doc comment examples and AppDelegate sample Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the raw String? formId parameter in the lifecycle handler closure with a FormContext struct. This keeps the callback signature stable as new fields are added (e.g. step, formType) without breaking existing integrations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove currentFormContext stored property from IAFPresentationManager. Each bridge event (formWillAppear, formDisappeared, openDeepLink) now carries its own formId/formName and threads it directly through the call chain, matching the Android SDK pattern. No local state is persisted between events. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merge master into feat/form-lifecycle-hooks. Adopts master's FormLifecycleEvent associated-value pattern (formId/formName/buttonLabel on each case) while keeping the branch's no-stored-state approach for context threading. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| /// The display name of the form. | ||
| public let formName: String? | ||
| } |
There was a problem hiding this comment.
FormContext struct is unused dead code
Low Severity
FormContext is a new public struct that isn't referenced anywhere in production code. The PR description states that methods take "an explicit FormContext parameter," but presentForm, dismissForm, and invokeLifecycleHandler all still use individual formId: String? and formName: String? parameters. This appears to be leftover from an incomplete refactor.
There was a problem hiding this comment.
If this is still around as dead code, then yes we should remove it
- Make formId, formName, and buttonLabel non-optional (String instead of String?) in FormLifecycleEvent, using empty string fallback when bridge data is missing - Move formShown/formDismissed callback invocation from IAFPresentationManager to bridge layer (IAFWebViewModel), matching formCtaClicked pattern - Remove dismiss safety net (hasInvokedDismissed guard and destroyWebView fallback) since dismiss is now purely bridge-driven - Remove in-memory form state (lastPresentedFormId/lastPresentedFormName) and associated fallback logic from IAFPresentationManager - Update tests and docstring examples to reflect non-optional types Part of MAGE-287, aligns with Android SDK PR #414 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The formShown and formDismissed doc comments claimed behavior that no longer matches the implementation. formShown now accurately describes that it reflects the JS-side formWillAppear signal rather than firing after native validation. formDismissed clarifies it does not fire for pre-show scenarios like session timeouts or aborts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FormCtaClicked.deepLinkUrl is now URL (non-optional). If the bridge message has no deep link URL configured, the CTA lifecycle event is not emitted — there's nothing actionable for the host app. Part of MAGE-287 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align form lifecycle events with Android SDK
| manager.invokeLifecycleHandler(for: .formCtaClicked( | ||
| formId: effectiveFormId, | ||
| formName: effectiveFormName, | ||
| buttonLabel: buttonLabel, |
There was a problem hiding this comment.
CTA lifecycle event lost when no deep link URL
High Severity
The formCtaClicked lifecycle event is no longer emitted when a CTA button has no deep link URL configured. The previous code explicitly fired this event unconditionally ("always fire, even if URL is nil/invalid") before the URL guard. Now the guard let url early-return on line 284 prevents invokeLifecycleHandler from ever being reached for URL-less CTAs. SDK consumers tracking all CTA clicks via registerFormLifecycleHandler will silently lose visibility into these interactions.
Reviewed by Cursor Bugbot for commit e90dbb9. Configure here.
There was a problem hiding this comment.
Intentional. If there's truly no iOS link, we don't hand the CTA click over to the host app for deep link handling either, so just matching that behavior.
| // Fall back to the context captured at present time if the bridge sends nil identifiers | ||
| // (e.g. fender rollback or companion PR not yet deployed) | ||
| let effectiveFormId = formId ?? currentFormId | ||
| let effectiveFormName = formName ?? currentFormName |
There was a problem hiding this comment.
Programmatic dismissals silently lose formDismissed lifecycle event
Medium Severity
destroyWebView previously checked whether a form was currently visible (presentingViewController != nil) and fired formDismissed as a safety net for timeout-based and programmatic dismissals. That safety net is removed. Now formDismissed is only emitted when the JS bridge sends formDisappeared, but destroyWebView nils the viewModel and viewController before the bridge can respond, so the event is silently lost. This affects session-timeout and API-key-change paths where a form is actively displayed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e90dbb9. Configure here.
There was a problem hiding this comment.
Intentional: the circumstances when iOS programmatically kills the webview preclude any actual form having been shown to the user, e.g. abort bc of network timeout.
Aligns eventName property with Swift enum case naming convention and cross-platform consistency (RN already uses camelCase). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ 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 053536d. Configure here.
| case .formCtaClicked: return "form_cta_clicked" | ||
| case .formShown: return "formShown" | ||
| case .formDismissed: return "formDismissed" | ||
| case .formCtaClicked: return "formCtaClicked" |
There was a problem hiding this comment.
Public eventName format silently changed from snake_case
Medium Severity
The public eventName property values changed from "form_shown", "form_dismissed", and "form_cta_clicked" to "formShown", "formDismissed", and "formCtaClicked". Since this is a public property on a public enum, any SDK consumer comparing or logging these string values (e.g., forwarding to third-party analytics) would silently get different values without any compile-time warning.
Reviewed by Cursor Bugbot for commit 053536d. Configure here.
2 participants
Summary
currentFormContextstored property fromIAFPresentationManager— no local form identity state is persisted between bridge eventsformDisappearedandopenDeepLinkbridge events now decode and carry their ownformId/formNamepayloads (previouslyformDisappeareddiscarded them entirely)formWillAppeareventRoot cause this fixes: All lifecycle callbacks (
form_shown,form_dismissed,form_cta_clicked) were showing the form version's internal name ("Default Popup") instead of the marketer-visible form name, becausecurrentFormContextwas set once byformWillAppear(which read from the wrong source) and reused for all subsequent events.Changes
IAFNativeBridgeEventformDisappearedandopenDeepLinkcases now carryformId/formName; addedFormDisappearedPayload; updatedDeepLinkEventPayloadIAFLifecycleEventdismisscase now carriesformId/formNameIAFWebViewModelIAFPresentationManagercurrentFormContext;presentForm,dismissForm, andinvokeLifecycleHandlerall take an explicitFormContextparameterTest plan
form_shown,form_dismissed, andform_cta_clickedall emit the correct marketer-visible form name (not "Default Popup")Part of klaviyo/fender#56023
🤖 Generated with Claude Code