Skip to content

Commit f7d631a

Browse files
evan-masseauclaude
evan-masseau
and
claude
committed
fix(forms): validate required bridge message fields (MAGE-484) (#437)
* fix(forms): throw on missing required fields in bridge message parsing Add a requireString() extension on JSONObject that throws IllegalArgumentException when a required field is missing or empty. The existing catch-and-log in KlaviyoNativeBridge.postMessage() prevents malformed events from propagating to the host app. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * test: verify warning log when CTA has no Android route The empty-android-route test verified that neither the lifecycle handler nor DeepLinking were invoked, but didn't verify the warning log fires. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: make FormDisappeared tolerant of missing fields to prevent stuck overlays FormDisappeared now uses optString instead of requireString so that dismiss() always fires even if the JS bridge sends malformed data. Also fixes test assertions to verify log level rather than exact message content, per project guidelines. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * refactor(forms): move lifecycle validation from bridge parsing to event dispatch Parsing now always uses optString() with empty defaults so SDK-internal actions (present overlay, dismiss, handle deep links) are never blocked by missing metadata. Validation of formId/formName/buttonLabel is done in KlaviyoNativeBridge's show/close/deepLink methods right before dispatching FormLifecycleEvent to the host app callback. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address PR review nits — remove redundant optString defaults, use level-only log assertions Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(forms): allow empty buttonLabel on CTA events A CTA button with no text label is still a valid click event. Only formId and formName are required non-empty for the lifecycle callback. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * docs(forms): fix lifecycle event timing documentation Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(forms): improve test validity and assertions Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]> * docs(forms): remove internal implementation details from public API docs Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
1 parent acf8b8f commit f7d631a

6 files changed

Lines changed: 207 additions & 74 deletions

File tree

‎sdk/forms-core/src/main/java/com/klaviyo/forms/FormLifecycleEvent.kt‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,33 @@ sealed interface FormLifecycleEvent {
2323

2424
/**
2525
* Triggered when a form is shown to the user.
26+
*
27+
* Fired after the SDK has initiated form presentation.
2628
*/
2729
data class FormShown(
2830
override val formId: String,
2931
override val formName: String
3032
) : FormLifecycleEvent
3133

3234
/**
33-
* Triggered when a form is dismissed (closed) by the user.
35+
* Triggered when a form is dismissed by the user.
36+
*
37+
* Fired after the SDK has initiated form dismissal. Fires for
38+
* user-initiated dismissals (e.g. tapping outside, close button).
39+
* Does not fire when the SDK tears down the form internally
40+
* (session timeouts, aborts).
3441
*/
3542
data class FormDismissed(
3643
override val formId: String,
3744
override val formName: String
3845
) : FormLifecycleEvent
3946

4047
/**
41-
* Triggered when a user taps a call-to-action (CTA) button in the form.
48+
* Triggered when a user taps a call-to-action (CTA) button in a form
49+
* that has a deep link URL configured.
50+
*
51+
* Fired after the SDK has initiated deep link navigation. Not emitted
52+
* if no deep link URL is configured for the CTA.
4253
*
4354
* @property buttonLabel The text label of the CTA button.
4455
* @property deepLinkUrl The deep link URI configured for the CTA.

‎sdk/forms-core/src/main/java/com/klaviyo/forms/FormLifecycleHandler.kt‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ package com.klaviyo.forms
66
* Implement this interface to receive notifications when in-app form lifecycle events occur.
77
*
88
* **Threading:** The handler is always invoked on the main thread.
9-
* Avoid performing long-running or blocking work in this handler, as it may delay
10-
* form presentation or dismissal.
9+
* Lifecycle callbacks fire after the SDK has already initiated the corresponding
10+
* action (presentation, dismissal, or deep link navigation). Avoid performing
11+
* long-running or blocking work in this handler, as it runs on the main thread.
1112
*
1213
* Example usage:
1314
* ```

‎sdk/forms/src/main/java/com/klaviyo/forms/bridge/KlaviyoNativeBridge.kt‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,14 @@ internal class KlaviyoNativeBridge : NativeBridge {
9696
*/
9797
private fun show(bridgeMessage: FormWillAppear) {
9898
Registry.get<PresentationManager>().present()
99+
100+
if (bridgeMessage.formId.isEmpty() || bridgeMessage.formName.isEmpty()) {
101+
Registry.log.warning(
102+
"FormWillAppear missing required fields, skipping lifecycle callback"
103+
)
104+
return
105+
}
106+
99107
invokeFormLifecycleHandler(
100108
FormLifecycleEvent.FormShown(bridgeMessage.formId, bridgeMessage.formName)
101109
)
@@ -129,6 +137,14 @@ internal class KlaviyoNativeBridge : NativeBridge {
129137
}
130138

131139
DeepLinking.handleDeepLink(deepLinkUri)
140+
141+
if (message.formId.isEmpty() || message.formName.isEmpty()) {
142+
Registry.log.warning(
143+
"OpenDeepLink missing required fields, skipping lifecycle callback"
144+
)
145+
return
146+
}
147+
132148
invokeFormLifecycleHandler(
133149
FormLifecycleEvent.FormCtaClicked(
134150
formId = message.formId,
@@ -144,6 +160,14 @@ internal class KlaviyoNativeBridge : NativeBridge {
144160
*/
145161
private fun close(bridgeMessage: FormDisappeared) {
146162
Registry.get<PresentationManager>().dismiss()
163+
164+
if (bridgeMessage.formId.isEmpty() || bridgeMessage.formName.isEmpty()) {
165+
Registry.log.warning(
166+
"FormDisappeared missing required fields, skipping lifecycle callback"
167+
)
168+
return
169+
}
170+
147171
invokeFormLifecycleHandler(
148172
FormLifecycleEvent.FormDismissed(bridgeMessage.formId, bridgeMessage.formName)
149173
)

‎sdk/forms/src/test/java/com/klaviyo/forms/FormLifecycleHandlerTest.kt‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -154,20 +154,25 @@ internal class FormLifecycleHandlerTest : BaseTest() {
154154
@Test
155155
fun `callback is not invoked when no callback is registered`() {
156156
// Simulate form shown message without registering a callback
157-
val message = """{"type":"formWillAppear", "data":{"formId":"$testFormId"}}"""
157+
val message = """{"type":"formWillAppear", "data":{"formId":"$testFormId","formName":"$testFormName"}}"""
158158
nativeBridge.postMessage(message)
159159

160160
// Verify PresentationManager was called but no exception thrown
161161
verify { mockPresentationManager.present() }
162162
}
163163

164164
@Test
165-
fun `formDisappeared without data delegates dismiss with empty context fields`() {
166-
Klaviyo.registerFormLifecycleHandler(FormLifecycleHandler { _ -> })
165+
fun `formDisappeared without data still dismisses but skips lifecycle callback`() {
166+
var callbackInvoked = false
167+
Klaviyo.registerFormLifecycleHandler(FormLifecycleHandler { _ -> callbackInvoked = true })
167168

168169
nativeBridge.postMessage("""{"type":"formDisappeared"}""")
169170

171+
// FormDisappeared uses tolerant parsing so dismiss always fires
170172
verify { mockPresentationManager.dismiss() }
173+
// Missing formId/formName means the lifecycle callback should be skipped
174+
assert(!callbackInvoked) { "Lifecycle callback should not fire when form metadata is missing" }
175+
verify { spyLog.warning(any()) }
171176
}
172177

173178
@Test
@@ -178,7 +183,7 @@ internal class FormLifecycleHandlerTest : BaseTest() {
178183
Klaviyo.registerFormLifecycleHandler(FormLifecycleHandler { _ -> })
179184

180185
nativeBridge.postMessage(
181-
"""{"type":"openDeepLink","data":{"android":"https://example.com","formId":"$testFormId"}}"""
186+
"""{"type":"openDeepLink","data":{"android":"https://example.com","formId":"$testFormId","formName":"$testFormName","buttonLabel":"Shop Now"}}"""
182187
)
183188

184189
verify { mockThreadHelper.runOnUiThread(any()) }

‎sdk/forms/src/test/java/com/klaviyo/forms/bridge/KlaviyoNativeBridgeTest.kt‎

Lines changed: 84 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -114,21 +114,43 @@ internal class KlaviyoNativeBridgeTest : BaseTest() {
114114
}
115115

116116
@Test
117-
fun `formWillAppear triggers show`() {
117+
fun `formWillAppear with no data still presents but skips lifecycle callback`() {
118118
/**
119119
* @see com.klaviyo.forms.bridge.KlaviyoNativeBridge.show
120120
*/
121+
val mockLifecycleHandler = mockk<FormLifecycleHandler>(relaxed = true)
122+
Registry.register<FormLifecycleHandler>(mockLifecycleHandler)
123+
121124
postMessage("""{"type":"formWillAppear"}""")
122125
verify { mockPresentationManager.present() }
126+
verify(exactly = 0) { mockLifecycleHandler.onFormLifecycleEvent(any()) }
127+
verify { spyLog.warning(any()) }
128+
129+
Registry.unregister<FormLifecycleHandler>()
123130
}
124131

125132
@Test
126133
fun `formWillAppear triggers show with IDs`() {
127134
/**
128135
* @see com.klaviyo.forms.bridge.KlaviyoNativeBridge.show
129136
*/
130-
postMessage("""{"type":"formWillAppear", "data":{"formId":"64CjgW"}}""")
137+
postMessage(
138+
"""{"type":"formWillAppear", "data":{"formId":"64CjgW","formName":"Test Form"}}"""
139+
)
140+
verify { mockPresentationManager.present() }
141+
}
142+
143+
@Test
144+
fun `formWillAppear with missing formId still presents but skips lifecycle callback`() {
145+
val mockLifecycleHandler = mockk<FormLifecycleHandler>(relaxed = true)
146+
Registry.register<FormLifecycleHandler>(mockLifecycleHandler)
147+
148+
postMessage("""{"type":"formWillAppear","data":{"formName":"Test Form"}}""")
131149
verify { mockPresentationManager.present() }
150+
verify(exactly = 0) { mockLifecycleHandler.onFormLifecycleEvent(any()) }
151+
verify { spyLog.warning(any()) }
152+
153+
Registry.unregister<FormLifecycleHandler>()
132154
}
133155

134156
@Test
@@ -264,7 +286,8 @@ internal class KlaviyoNativeBridgeTest : BaseTest() {
264286
"ios": "klaviyotest://settings",
265287
"android": "klaviyotest://settings",
266288
"formId": "64CjgW",
267-
"formName": "Test Form"
289+
"formName": "Test Form",
290+
"buttonLabel": "Click Me"
268291
}
269292
}
270293
""".trimIndent()
@@ -290,7 +313,10 @@ internal class KlaviyoNativeBridgeTest : BaseTest() {
290313
"type": "openDeepLink",
291314
"data": {
292315
"ios": "klaviyotest://settings",
293-
"android": ""
316+
"android": "",
317+
"formId": "64CjgW",
318+
"formName": "Test Form",
319+
"buttonLabel": "Click Me"
294320
}
295321
}
296322
""".trimIndent()
@@ -305,6 +331,31 @@ internal class KlaviyoNativeBridgeTest : BaseTest() {
305331

306332
verify(exactly = 0) { mockLifecycleHandler.onFormLifecycleEvent(any()) }
307333
verify(exactly = 0) { DeepLinking.handleDeepLink(any<Uri>()) }
334+
verify { spyLog.warning(any()) }
335+
336+
Registry.unregister<FormLifecycleHandler>()
337+
}
338+
339+
@Test
340+
fun `openDeepLink with missing buttonLabel still navigates and fires lifecycle callback`() {
341+
val message = """{"type":"openDeepLink","data":{"android":"klaviyotest://settings","formId":"64CjgW","formName":"Test Form"}}"""
342+
343+
mockkObject(DeepLinking)
344+
every { DeepLinking.handleDeepLink(any<Uri>()) } returns Unit
345+
346+
val events = mutableListOf<FormLifecycleEvent>()
347+
val callback = FormLifecycleHandler { event -> events.add(event) }
348+
Registry.register<FormLifecycleHandler>(callback)
349+
350+
postMessage(message)
351+
352+
verify { DeepLinking.handleDeepLink(mockUri) }
353+
assertEquals(1, events.size)
354+
val ctaEvent = events[0] as FormLifecycleEvent.FormCtaClicked
355+
assertEquals("64CjgW", ctaEvent.formId)
356+
assertEquals("Test Form", ctaEvent.formName)
357+
assertEquals("", ctaEvent.buttonLabel)
358+
assertEquals(mockUri, ctaEvent.deepLinkUrl)
308359

309360
Registry.unregister<FormLifecycleHandler>()
310361
}
@@ -335,7 +386,7 @@ internal class KlaviyoNativeBridgeTest : BaseTest() {
335386

336387
// CTA — formId+formName come from the bridge message itself
337388
postMessage(
338-
"""{"type":"openDeepLink","data":{"android":"klaviyotest://settings","formId":"abc","formName":"My Form"}}"""
389+
"""{"type":"openDeepLink","data":{"android":"klaviyotest://settings","formId":"abc","formName":"My Form","buttonLabel":"Shop Now"}}"""
339390
)
340391
assertEquals(3, events.size)
341392
val ctaEvent = events[2] as FormLifecycleEvent.FormCtaClicked
@@ -356,12 +407,36 @@ internal class KlaviyoNativeBridgeTest : BaseTest() {
356407
}
357408

358409
@Test
359-
fun `formDisappeared triggers dismiss with empty context when no data`() {
410+
fun `formDisappeared with no data still dismisses but skips lifecycle callback`() {
360411
/**
412+
* All message types use tolerant parsing (optString) so that SDK-internal actions
413+
* always fire, preventing the user from getting stuck behind a full-screen overlay.
414+
* Lifecycle callbacks are gated on valid metadata at the dispatch layer.
415+
*
361416
* @see com.klaviyo.forms.bridge.KlaviyoNativeBridge.close
362417
*/
418+
val mockLifecycleHandler = mockk<FormLifecycleHandler>(relaxed = true)
419+
Registry.register<FormLifecycleHandler>(mockLifecycleHandler)
420+
363421
postMessage("""{"type":"formDisappeared"}""")
364422
verify { mockPresentationManager.dismiss() }
423+
verify(exactly = 0) { mockLifecycleHandler.onFormLifecycleEvent(any()) }
424+
verify { spyLog.warning(any()) }
425+
426+
Registry.unregister<FormLifecycleHandler>()
427+
}
428+
429+
@Test
430+
fun `formDisappeared with missing formId still dismisses but skips lifecycle callback`() {
431+
val mockLifecycleHandler = mockk<FormLifecycleHandler>(relaxed = true)
432+
Registry.register<FormLifecycleHandler>(mockLifecycleHandler)
433+
434+
postMessage("""{"type":"formDisappeared","data":{"formName":"Test Form"}}""")
435+
verify { mockPresentationManager.dismiss() }
436+
verify(exactly = 0) { mockLifecycleHandler.onFormLifecycleEvent(any()) }
437+
verify { spyLog.warning(any()) }
438+
439+
Registry.unregister<FormLifecycleHandler>()
365440
}
366441

367442
@Test
@@ -378,36 +453,19 @@ internal class KlaviyoNativeBridgeTest : BaseTest() {
378453
@Test
379454
fun `malformed message throws an error`() {
380455
postMessage("sawr a warewolf with a chinese menu inhis hands")
381-
382-
verify {
383-
spyLog.error(
384-
"Failed to relay webview message: sawr a warewolf with a chinese menu inhis hands",
385-
any<JSONException>()
386-
)
387-
}
456+
verify { spyLog.error(any(), any<JSONException>()) }
388457
}
389458

390459
@Test
391460
fun `unknown type throws an error`() {
392461
postMessage("""{"type":"unknown"}""")
393-
394-
verify {
395-
spyLog.error(
396-
"Failed to relay webview message: {\"type\":\"unknown\"}",
397-
any<IllegalStateException>()
398-
)
399-
}
462+
verify { spyLog.error(any(), any<IllegalStateException>()) }
400463
}
401464

402465
@Test
403466
fun `null message logs warning`() {
404467
postMessage(null)
405468

406-
verify {
407-
spyLog.warning(
408-
"Received null message from webview",
409-
null
410-
)
411-
}
469+
verify { spyLog.warning(any(), null) }
412470
}
413471
}

0 commit comments

Comments
 (0)