Skip to content

[Remote Config] Add Custom Signals - #1899

Merged
a-maurice merged 20 commits into
mainfrom
am-custom_signals
Aug 18, 2026
Merged

[Remote Config] Add Custom Signals#1899
a-maurice merged 20 commits into
mainfrom
am-custom_signals

Conversation

@a-maurice

Copy link
Copy Markdown
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

Add support for setting Custom Signals to Remote Config. This also adds some of the remote config unit tests back in.


Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Running integration tests and unit tests locally.


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread remote_config/src/android/remote_config_android.cc Outdated
Comment thread remote_config/src/ios/remote_config_ios.mm
Comment thread remote_config/src/desktop/remote_config_desktop.cc
@a-maurice

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread remote_config/src/android/remote_config_android.cc
Comment thread remote_config/src/ios/remote_config_ios.mm
@a-maurice

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread remote_config/src/desktop/rest.h
Comment thread remote_config/src/desktop/remote_config_desktop.cc
@a-maurice

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread remote_config/src/desktop/remote_config_request.cc
@a-maurice
a-maurice requested a review from AustinBenoit August 13, 2026 20:32
Comment thread remote_config/src/desktop/remote_config_desktop.cc Outdated
Comment thread remote_config/src/desktop/remote_config_desktop.cc Outdated
@a-maurice
a-maurice enabled auto-merge (squash) August 18, 2026 18:31
@a-maurice
a-maurice merged commit c28b251 into main Aug 18, 2026
47 of 49 checks passed
@github-actions github-actions Bot added the tests: in-progress This PR's integration tests are in progress. label Aug 18, 2026
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

Integration test with FLAKINESS (succeeded after retry)

Requested by @a-maurice on commit c28b251
Last updated: Tue Aug 18 14:38 PDT 2026
View integration test log & download artifacts

Failures Configs
remote_config [TEST] [FLAKINESS] [iOS] [macos] [1/2 ios_device: ios_target]
(1 failed tests)  FirebaseRemoteConfigTest.TestAddOnConfigUpdateListener

Add flaky tests to go/fpl-cpp-flake-tracker

@a-maurice
a-maurice deleted the am-custom_signals branch August 18, 2026 20:23
@github-actions github-actions Bot added the tests: succeeded This PR's integration tests succeeded. label Aug 18, 2026
@firebase-workflow-trigger firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests: succeeded This PR's integration tests succeeded.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants