[Remote Config] Add Custom Signals - #1899
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for setting Custom Signals in Firebase Remote Config across Android, iOS, and Desktop platforms, along with corresponding tests. The review feedback highlights three critical issues regarding type handling: on Android and iOS, passing a null Variant fails to clear the custom signal and instead logs an error or gets omitted; on Desktop, converting numeric custom signals to string representations breaks backend targeting rules that expect numeric types, so the original Variant types should be preserved.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for setting Custom Signals in Remote Config across Android, iOS, and Desktop platforms, allowing developers to set custom key-value pairs for targeting. Feedback on the changes highlights JNI robustness issues on Android, such as missing null/OOM checks, improper exception handling, and inconsistent error handling for invalid Variant types. Additionally, it is recommended that the iOS implementation fail the future when encountering an invalid Variant type to maintain consistency with the Desktop platform's behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for setting Custom Signals in Remote Config across Android, iOS, and Desktop platforms, including metadata serialization, REST request payload injection, and comprehensive testing. Feedback highlights two critical issues: first, RemoteConfigREST holds a copy of LayeredConfigs instead of a reference, which prevents updated custom signals from being propagated to the backend; second, the Desktop implementation merges custom signals while Android and iOS overwrite them, leading to platform inconsistencies and failing integration tests when clearing signals.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for setting Custom Signals in Firebase Remote Config across Android, iOS, and Desktop platforms. It introduces the public APIs SetCustomSignals and SetCustomSignalsLastResult, implements platform-specific JNI, Objective-C, and REST bindings, handles metadata serialization, and adds comprehensive unit and integration tests. A critical issue was found in remote_config_request.cc where the logical condition for generate_status is inverted due to an incorrect comparison with nullptr, which would cause the function to assert and return early on success.
Description
Add support for setting Custom Signals to Remote Config. This also adds some of the remote config unit tests back in.
Testing
Running integration tests and unit tests locally.
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.