feat: setEmbeddedData() + clearEmbeddedData() for app surveys [ENG-2472] - #58
Conversation
A host app can attach context to future responses without tying it to a
trigger. `track()` takes a name and nothing else, so today the only way to
get context onto a response is to declare it in the survey and have the
respondent type it.
Formbricks.setEmbeddedData(mapOf(
"screen" to EmbeddedDataValue.string("checkout"),
"plan" to EmbeddedDataValue.string("pro"),
))
Formbricks.setEmbeddedData(mapOf("screen" to null)) // remove one key
Formbricks.clearEmbeddedData("plan") // same, explicitly
Formbricks.clearEmbeddedData() // everything
Merge, never replace, so refreshing a volatile field cannot wipe a stable
one. `null` removes a key; a key left out is untouched, which is how a host
skips a field it has no value for this screen. The single-key and
clear-everything forms are separate overloads, so a non-null `String`
parameter means a host reading the key from its own state cannot
accidentally wipe the bag.
In-memory and never persisted: persisting would blur the Embedded Data ↔
contact-attribute boundary and create a PII-at-rest surface. Cleared on an
identity switch and on logout so one user's context cannot ride onto the
next user's responses on a shared device; kept on first identification,
because a host legitimately pushes context before it knows who the user is.
Callable before setup(), unlike every other public method: a host that
pushes context at launch must not have the value dropped because
initialization had not finished.
Snapshotted in loadHtml(), which runs when the survey is actually presented
after any configured delay, and frozen for its lifetime. The bag rides the
props payload that already exists, under `hiddenFieldsRecord` — no new
bridge message, and deliberately so: a setEmbeddedData after display must
not reach the survey on screen. It is passed raw and unfiltered, because the
ingest contract lives in the renderer and the server re-runs all of it.
A non-finite number is logged and skipped: it would serialize as a bare NaN
or Infinity, which is not valid JSON, so JSON.parse in the WebView would
throw and no survey would render at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018L6PrECN38Jbe1FEdFccTz
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
…e bag [ENG-2472] CI caught what no local check could: `UserManager.set(userId)` only enqueues into the debounced UpdateQueue, so `UserManager.userId` is still null immediately after `Formbricks.setUserId`. The switch test therefore took the first-identification branch, where the bag is kept on purpose — asserting an empty bag against a code path that never ran. It also flipped `isInitialized` by hand instead of setting the SDK up, so no sync could ever complete. The production code is right and stays as it is: the clearing sits inside the SDK's own "a different userId is set" branch, so it is exactly as timely as the `UserManager.logout()` teardown beside it. The tests were asserting a state the SDK cannot reach that fast. The identity cases now run against a real `Formbricks.setup` with the mock API service and wait for the id to land — the same pattern as the SDK's own identity tests — so the switch and same-id cases exercise the branches they name. Also wraps the shared_prefs probe's file read, so an unreadable file fails the read rather than the test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6PrECN38Jbe1FEdFccTz
…2472] Second red run on the emulator job, and the runner's log tail is entirely harden-runner output, so the failing assertion is not reachable from here. Rather than guess a third time, this removes the machinery the identity tests did not need. `Formbricks.setup` is gone from this class: it fetched the workspace and ran the legacy-cache migration, writing state that other test classes assert on, and none of it is needed to exercise an identity switch. The debounced wait is gone too — identity lands only when a network sync completes, so waiting tests the UpdateQueue's timing as much as the branch it names. Seeding the SharedPreferences key the getter falls back to is exact: no timer, no request, and it models the honest scenario — the app relaunches already identified, then a different user signs in. `setUserId` then genuinely takes the switch branch, and the same-id case genuinely takes the early return. `appUrl`/`workspaceId` are assigned directly in setUp because they are lateinit: a queued update that found them unset would throw on the UpdateQueue's timer thread, and a throwing TimerTask cancels that Timer for the whole process — the same hazard the SDK's own comments call out. An @after now logs out and resets the queue so nothing this class starts can fire during another one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6PrECN38Jbe1FEdFccTz
|
Two things from manual testing + review:
|
…NG-2472] Two review findings. 1. `Date.dateString()` built its SimpleDateFormat with Locale.getDefault(), and SimpleDateFormat renders digits in the locale's own numbering system. On a device set to fa, fa-IR, ar-EG, hi-IN-u-nu-deva or ne-NP the "ISO 8601" string came out in non-ASCII digits, which the ingest contract's date parser will not accept - the value would be stored raw and flagged coercion_failed, for those users only. Locale.ROOT fixes it. Verified on the JVM: six locales produce non-ASCII digits with getDefault() and ASCII with ROOT, and the new test's regex accepts the ROOT output while rejecting both native-digit ones. Only the formatting direction needed it. The three parsers in that file keep Locale.getDefault(): checked, and DecimalFormat's Character.digit fallback reads the server's ASCII digits under fa and ar-EG alike, so they are not a live bug and are left alone. The pre-existing date assertion compared dateString() against dateString(), so it could not have caught this. The new test asserts the shape instead. 2. setEmbeddedData succeeded in silence. Mirrors the js-core debug trace from formbricks/formbricks#9091: keys set and removed, what the bag now holds, and the sentence that pre-empts the next question. Keys only, never values - the documented use of this bag includes hashed identity fields - and the message is built by an internal `setTrace` so that property is directly assertable rather than scraped from logcat. Built and logged outside the lock, so a log write never holds it. :android:compileDebugKotlin and :android:compileDebugAndroidTestKotlin both clean. The instrumented suite still runs only in CI; no emulator here.
|
Both done in 2. So six locales, not just Scope, since Not a live bug, so I left them and said so in a comment on Worth flagging: the fix also touches The existing test could not have caught this — 1. Debug trace. Mirrors formbricks/formbricks#9091 via
Generated by Claude Code |
|
3 participants
What & why
Was:
track()takes a name and nothing else, so the only way to get context onto an app-survey response was to declare a field and have the respondent type it. Now: the host app can attach context to future responses without tying it to a trigger.Mirrors js-core's
setEmbeddedDatakey for key (formbricks/formbricks#8989), so web and mobile behave identically.nullremoves a key; a key left out is untouched — that is how a host skips a field it has no value for this screen. The single-key and clear-everything forms are separate overloads, so a non-nullStringparameter means a host reading the key from its own state cannot accidentally wipe the bag.SharedPreferences— persisting would blur the Embedded Data ↔ contact-attribute boundary and create a PII-at-rest surface. Cleared on an identity switch and onlogout, so one user's context cannot ride onto the next user's responses on a shared device; kept on first identification, because a host legitimately pushes context before it knows who the user is.setup(), unlike every other public method: a host that pushes context at launch must not have the value dropped because initialization had not finished.loadHtml(), which runs after any configured delay.hiddenFieldsRecord— no new bridge message. It goes out raw: the ingest contract (allow-list, coercion,locked, size caps) lives in the renderer, and the server re-runs all of it.Where to look:
manager/EmbeddedDataManager.kt(the store, the lock, the trace and every lifetime rule) ·Formbricks.kt(the two overloads and identity-switch clearing) ·extensions/DateExtensions.kt(the locale fix) ·webview/FormbricksViewModel.kt(one key in the payload).Requires the renderer change in formbricks/formbricks#9067 for the auto-capture half; this PR is independent of it and needs no server change.
Linear ticket
https://linear.app/formbricks/issue/ENG-2472/mobile-sdk-parity-for-embedded-data-one-batched-release-per-sdk
How this was tested
./gradlew :android:compileDebugKotlin✅ and:android:compileDebugAndroidTestKotlin✅ (Android SDK 35, JDK 21), re-run after each change. No new warnings./dev/kvmis absent. CI's emulator job is the only place these execute. Anything that could be verified without one was verified on the JVM instead; see the folds.androidTest/.../manager/EmbeddedDataManagerInstrumentedTest.kt(20) covers merge,nullremoval, omitted-key no-op, last-write-wins, single-key clear, clearing an unset key, clear-all, every scalar withDate→ISO 8601, ASCII ISO 8601 under a native-digit device locale, JSON parseability (including a value containing quotes), the non-finite-number guard, the detached snapshot, works-before-setup, not-persisted-to-SharedPreferences, the four identity cases, the two success-trace cases, and 1600 concurrent writes against the lock.The locale bug, and why the old test could not have caught it
Date.dateString()built itsSimpleDateFormatwithLocale.getDefault(), andSimpleDateFormatrenders digits in the locale's own numbering system. Reproduced on the JVM against the exact pattern:On those devices the "ISO 8601" string came out in non-ASCII digits, which the ingest contract's date parser will not accept — an Embedded Data
datefield would be stored raw and flaggedcoercion_failed, for those users only.Locale.ROOTfixes it.Scope. Only
dateString()changed. The other three formatters in that file parse server-sent ISO strings, andDecimalFormat'sCharacter.digitfallback reads ASCII digits underfaandar-EGalike — verified — so they are not a live bug and were left alone, with a comment ondateStringsaying why. The one-line fix also coversUserManager'sDisplay(surveyId, lastDisplayedAt.dateString()), which had the same hazard and predates this PR.The pre-existing assertion was
assertEquals(signedUpAt.dateString(), json.get("signedUpAt").asString)—dateString()compared to itself, so it passed under any locale. The new test asserts the shape instead, underLocale("fa")andLocale.forLanguageTag("ar-EG-u-nu-arab")(the latter forces the numbering system rather than relying on what ICU picks), restoring the default in afinally. Verified on the JVM that its regex accepts theROOToutput and rejects both native-digit ones, i.e. it genuinely goes red on the old code.Why the identity tests were rewritten twice
The defect.
UserManager.set(userId)only enqueues into the debouncedUpdateQueue;UserManager.userIdis written when the sync completes. So immediately afterFormbricks.setUserId("user-a")that property is still null, and the followingsetUserId("user-b")took the first-identification branch — where the bag is deliberately kept. The test was asserting an empty bag against a code path that never ran. It also flippedisInitializedby hand instead of setting the SDK up, so no sync could ever have completed anyway.The first fix stood the SDK up for real (
Formbricks.setup+MockFormbricksApiService) and waited 2s for the id to land. Still red — and the runner's log tail is entirelyharden-runnernetwork output, so the failing assertion is not reachable through the API from here. Rather than guess a third time, the current shape removes the machinery those tests never needed:Formbricks.setupis gone from this class. It fetched the workspace and ran the legacy-cache migration, writing state that other test classes assert on — a plausible source of cross-class breakage that has nothing to do with what these tests check.SharedPreferenceskey the getter falls back to: exact, no timer, no request, and it models the honest scenario — the app relaunches already identified, then a different user signs in.setUserIdthen genuinely takes the switch branch, and the same-id case genuinely takes the early return.appUrl/workspaceIdare assigned directly insetUpbecause they arelateinit: a queued update finding them unset would throw on theUpdateQueue's timer thread, and a throwingTimerTaskcancels thatTimerfor the whole process. An@Afterlogs out and resets the queue so nothing this class starts can fire during another one.The production code is unchanged throughout. The clearing sits inside the SDK's own
if (existing != null && existing.isNotEmpty())branch, so it is exactly as timely as theUserManager.logout()teardown beside it.Why a non-finite number is refused at the door
Gson writes a bare
NaN/InfinityintoJsonObject.toString(), which is not valid JSON. The payload it lands in is the whole survey's props blob, parsed in the WebView withJSON.parseat the top of the template with notry/catcharound it — so a singlesetEmbeddedData(mapOf("x" to EmbeddedDataValue.number(Double.NaN)))from host code would mean no survey, not a missing field. The store drops it with a log instead, and a test asserts the snapshot still parses.Review follow-ups (commit
bdeb6fc)Logger.d(already gated onFormbricks.loggingEnabled). The message is built by aninternal setTracerather than inlined, so "keys only, never values" is assertable directly instead of by scraping logcat — the only other option in an instrumented suite, and a flaky one. Built and logged outsidesynchronized, so a log write never holds the lock.Breaking changes
None. Three new methods on the
Formbricksobject, one new publicEmbeddedDataValuesealed class, and one new key in a payload the renderer already accepts.setup,track,setUserId,setAttribute(s),setLanguageandlogoutkeep their signatures;logout()and an identity-switchingsetUserId()additionally clear the new in-memory bag, which did not exist before.Date.dateString()now formats withLocale.ROOT. Its output is a machine-facing ISO 8601 string that is only ever sent on the wire, and on every locale that was already ASCII the bytes are identical — the change is only visible on the native-digit locales listed above, where the previous output was rejected downstream.QA / Test Plan
How to test
planon an app survey. CallFormbricks.setEmbeddedData(mapOf("plan" to EmbeddedDataValue.string("pro"))), thenFormbricks.track()the survey's action → the response showsplan = pro.plan, then setscreenin a second call → the response carries both. Merge, not replace.setEmbeddedData(mapOf("plan" to null))→planis absent from the next response,screenis still there.clearEmbeddedData()with no argument → the next response carries none of the fields;clearEmbeddedData("plan")removes only that one.Datevalue → it arrives on the response as an ISO 8601 string in ASCII digits and reads back as a date on adate-typed field. Before this PR's follow-up commit it arrived in Persian/Arabic digits and was flaggedcoercion_failed. This is the highest-value step here — it only ever reproduced on those devices.setEmbeddedData/clearEmbeddedData→ each call logs the keys it set or removed and what the bag now holds. No values appear in any line. With logging disabled, nothing is logged.setEmbeddedDatawhile it is on screen, finish it → the response records the value from when the survey appeared.setUserId("a")→ wait for the sync to land → set values →setUserId("b")→ survey → the response carries none of them. Waiting matters: identity is debounced, so a back-to-back switch is not yet a switch to the SDK.logout()after setting values → the next response carries none of them.EmbeddedDataValue.number(Double.NaN)from host code → logged and skipped, and the survey still renders. This is the one that would fail loudly if the guard were missing."or</script>→ survives to the response intact and does not break the WebView (the payload is base64-encoded, so this should be unaffected — confirm).Preconditions / test data
surveys.umd.cjs, and a host app. Response card is the readout.Risks & regressions
UserManager.userId, which lands after the debounced/usersync — so asetUserId("a"); setUserId("b")in the same tick clears neither the user state nor the bag. Pre-existing behaviour, not new here, but it is what to expect when testing.logout()and identity-switchingsetUserId()now also clear the bag. Nothing else observes it, so no other behaviour changes.Date.dateString()is shared withUserManager's display records. The locale fix changes their bytes only on native-digit locales, where they were already wrong; worth a glance at display-based targeting on such a device.</script>.Migrations / env / cutover
Generated by Claude Code