Skip to content

fix: attribute data type updates#47

Open
pandeymangg wants to merge 10 commits intomainfrom
fix/attribute-data-type-updates
Open

fix: attribute data type updates#47
pandeymangg wants to merge 10 commits intomainfrom
fix/attribute-data-type-updates

Conversation

@pandeymangg
Copy link
Contributor

@pandeymangg pandeymangg commented Feb 16, 2026

setAttribute changes for supporting number, string and date attribute types. Also, adds a change which lets users override the current userId set by just calling the setUserId function again

@pandeymangg pandeymangg marked this pull request as ready for review February 16, 2026 08:55
@pandeymangg pandeymangg requested a review from Dhruwang February 16, 2026 08:56
@coderabbitai
Copy link

coderabbitai bot commented Feb 16, 2026

Walkthrough

This change introduces a new AttributeValue sealed class to provide type-safe handling of user attributes as either strings or numbers, replacing previous untyped string/any mappings throughout the codebase. Corresponding method signatures in Formbricks, UserManager, UpdateQueue, FormbricksConfig, and related classes are updated to use Map<String, AttributeValue> instead of Map<String, String>. Additionally, the SurveyOverlay abstraction is removed from survey display logic, simplifying the showSurvey API and related fragment handling. Error and message logging is enhanced in user response handling, HTTPS validation is added during setup, and userId setter logic is refined to handle no-op cases.

🚥 Pre-merge checks | ✅ 1 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Merge Conflict Detection ⚠️ Warning ❌ Merge conflicts detected (9 files):

⚔️ android/src/androidTest/java/com/formbricks/android/FormbricksInstrumentedTest.kt (content)
⚔️ android/src/main/java/com/formbricks/android/Formbricks.kt (content)
⚔️ android/src/main/java/com/formbricks/android/api/FormbricksApi.kt (content)
⚔️ android/src/main/java/com/formbricks/android/helper/FormbricksConfig.kt (content)
⚔️ android/src/main/java/com/formbricks/android/manager/SurveyManager.kt (content)
⚔️ android/src/main/java/com/formbricks/android/manager/UserManager.kt (content)
⚔️ android/src/main/java/com/formbricks/android/model/user/PostUserBody.kt (content)
⚔️ android/src/main/java/com/formbricks/android/model/user/UserResponseData.kt (content)
⚔️ android/src/main/java/com/formbricks/android/network/queue/UpdateQueue.kt (content)

These conflicts must be resolved before merging into main.
Resolve conflicts locally and push changes to this branch.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a description explaining the purpose of the attribute data type changes and their impact on the API.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: attribute data type updates' accurately reflects the main change: introducing AttributeValue wrapper type for attributes across the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
android/src/main/java/com/formbricks/android/Formbricks.kt (1)

127-183: ⚠️ Potential issue | 🟡 Minor

The (attribute, key) parameter order is confusing — value-before-key is the opposite of almost every key-value API convention.

The String overload's signature is pre-existing so renaming it now would be a breaking change, but the new KDoc examples (lines 132, 150, 169) and the two new overloads make this unusual ordering more visible. The natural expectation for callers is setAttribute(key, value), and the reversed order is an easy source of silent bugs — especially for the setAttribute(String, String) overload where both parameters have the same type and swapping them compiles without error.

Consider deprecating the current signature and introducing a (key, value) replacement to align with standard conventions before the new overloads get wider adoption.

🧹 Nitpick comments (5)
android/src/main/java/com/formbricks/android/network/queue/UpdateQueue.kt (2)

41-51: Redundant startDebounceTimer() call on line 46.

addAttribute() on line 45 already calls startDebounceTimer() internally (line 38). The explicit call on line 46 is redundant — it cancels and recreates the timer unnecessarily.

♻️ Suggested fix
     fun setLanguage(language: String) {
         val effectiveUserId = userId ?: UserManager.userId
 
         if (effectiveUserId != null) {
             addAttribute("language", AttributeValue.string(language))
-            startDebounceTimer()
         } else {
             Logger.d("UpdateQueue - updating language locally: $language")
             return
         }
     }

20-20: language field appears to be dead code.

The language field is declared on line 20 and reset to null in reset() (line 56), but it's never assigned a meaningful value or read anywhere in this class. Consider removing it.

#!/bin/bash
# Verify that the language field in UpdateQueue is never meaningfully used
rg -n "this\.language\b|UpdateQueue\.language" --type=kt -C2
android/src/main/java/com/formbricks/android/manager/UserManager.kt (1)

147-155: Good addition of server response logging.

Logging errors and informational messages from the user response is valuable for debugging attribute-related issues (e.g., type mismatches, invalid keys).

One minor note: wrapping each error string in RuntimeException(error) on line 149 may produce unnecessary stack traces in logs depending on Logger.e's implementation. If Logger has a string-based overload, consider using it to avoid noisy stack traces for what are essentially server-provided validation messages.

android/src/main/java/com/formbricks/android/helper/FormbricksConfig.kt (2)

47-65: setStringAttributes convenience is helpful, but addAttribute only handles strings.

setStringAttributes provides a smooth migration path for callers with string-only attributes. However, the addAttribute method only accepts String values — there's no equivalent for Double (or Date). Callers who want to add a single numeric attribute via the Builder must fall back to constructing the full map with setAttributes.

Consider adding an overload for parity:

Suggested addition
/**
 * Adds a numeric attribute to the Builder object.
 */
fun addAttribute(attribute: Double, key: String): Builder {
    this.attributes[key] = AttributeValue.number(attribute)
    return this
}

77-86: Prevent unnecessary attribute updates for empty maps.

Since attributes is initialized as mutableMapOf() on line 23, build() always produces a non-null map. In Formbricks.setup(), the config.attributes?.let { UserManager.setAttributes(it) } call will execute even with an empty map, potentially triggering wasteful UpdateQueue operations.

If empty attributes should be treated as "no attributes", update the build() method to return null for empty maps:

Suggested diff
 fun build(): FormbricksConfig {
     return FormbricksConfig(
         appUrl = appUrl,
         environmentId = environmentId,
         userId = userId,
-        attributes = attributes,
+        attributes = attributes.ifEmpty { null },
         loggingEnabled = loggingEnabled,
         fragmentManager = fragmentManager
     )
 }

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
54.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant