Conversation
WalkthroughThis change introduces a new 🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ 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. Comment |
There was a problem hiding this comment.
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 | 🟡 MinorThe
(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 thesetAttribute(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: RedundantstartDebounceTimer()call on line 46.
addAttribute()on line 45 already callsstartDebounceTimer()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:languagefield appears to be dead code.The
languagefield is declared on line 20 and reset tonullinreset()(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 -C2android/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 onLogger.e's implementation. IfLoggerhas 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:setStringAttributesconvenience is helpful, butaddAttributeonly handles strings.
setStringAttributesprovides a smooth migration path for callers with string-only attributes. However, theaddAttributemethod only acceptsStringvalues — there's no equivalent forDouble(orDate). Callers who want to add a single numeric attribute via the Builder must fall back to constructing the full map withsetAttributes.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
attributesis initialized asmutableMapOf()on line 23,build()always produces a non-null map. InFormbricks.setup(), theconfig.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 returnnullfor 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 ) }
|


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