Repository navigation
Conversation
…s key `RemoteConfigValueObservable`, which backs `@RemoteConfigProperty`, starts with the key's remote or in-app default value, or with the wrapper's fallback value if the key has neither. On activation, though, it only updated its value if the key still had a remote value. If the activated template no longer contained the key, the property kept the previous remote value instead of the default or fallback value that Remote Config itself now returns, so SwiftUI views showed stale configuration. Use the same remote/default/fallback selection on activation as at initialization. As at initialization, a value that can't be decoded now falls back to the fallback value instead of keeping the previous value. Fixes #16816
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates @RemoteConfigProperty to correctly use the in-app default or fallback value when an activation removes its key. The logic for updating the configuration value has been refactored into a helper method updateConfigValue() in RemoteConfigValueObservable, and a new unit test has been added to verify this behavior. The review feedback suggests renaming the local variable configValue within updateConfigValue() to remoteValue to avoid shadowing the class property self.configValue, which improves code readability and safety.
| private func updateConfigValue() { | ||
| do { | ||
| let configValue: RemoteConfigValue = remoteConfig[key] | ||
| if configValue.source == .remote { | ||
| if configValue.source == .remote || configValue.source == .default { | ||
| self.configValue = try configValue.decoded() | ||
| } else { | ||
| self.configValue = fallbackValue | ||
| } | ||
| } catch { | ||
| // Suppresses a hard failure if decoding failed. | ||
| configValue = fallbackValue | ||
| } | ||
| } |
There was a problem hiding this comment.
The local variable configValue shadows the class property self.configValue of the same name but with a different type (RemoteConfigValue vs T). This makes the code harder to read and maintain, especially in the catch block where configValue refers to self.configValue because the local variable is out of scope.
Renaming the local variable to remoteValue and using self.configValue consistently improves clarity and prevents potential bugs during future refactoring.
| private func updateConfigValue() { | |
| do { | |
| let configValue: RemoteConfigValue = remoteConfig[key] | |
| if configValue.source == .remote { | |
| if configValue.source == .remote || configValue.source == .default { | |
| self.configValue = try configValue.decoded() | |
| } else { | |
| self.configValue = fallbackValue | |
| } | |
| } catch { | |
| // Suppresses a hard failure if decoding failed. | |
| configValue = fallbackValue | |
| } | |
| } | |
| private func updateConfigValue() { | |
| do { | |
| let remoteValue: RemoteConfigValue = remoteConfig[key] | |
| if remoteValue.source == .remote || remoteValue.source == .default { | |
| self.configValue = try remoteValue.decoded() | |
| } else { | |
| self.configValue = fallbackValue | |
| } | |
| } catch { | |
| self.configValue = fallbackValue | |
| } | |
| } |
There was a problem hiding this comment.
[paulb777-bot] Done in fdadf24. SwiftFormat then removed the now-redundant self., so the helper assigns configValue directly.
Rename the local RemoteConfigValue to remoteValue so that it no longer shadows the configValue property, as suggested in review. No behavior change.
Problem
RemoteConfigValueObservablebacks@RemoteConfigProperty. When it's created, it uses the key'sremote or in-app default value, and the wrapper's
fallbackvalue if the key has neither. Onactivation, though,
configDidActivateonly updated the value if the key still had a remote value.Activation replaces the whole active config. So when the newly activated template no longer contains
the key (for example, the parameter was deleted or set to "Use in-app default", the scenario
reported in #11095),
RemoteConfigreturns the in-app default value (or a static value), but theproperty kept the previous remote value. SwiftUI views showed stale configuration until the view's
state object was recreated, e.g. after a relaunch.
Change
initintoupdateConfigValue()and call it from bothinitandconfigDidActivate. Activation now uses remote, then in-app default, then fallback, likeinitdoes. That matches the documented
fallbacksemantics: "The value to fall back to if the keydoesn't exist in remote or default configs".
value instead of keeping the previous value.
PropertyWrapperTests.testActivationThatRemovesKeyUsesDefaultOrFallback. It activates atemplate with two keys, one with an in-app default and one without, then activates a template
without them. It creates
RemoteConfigValueObservabledirectly (hence@testable import),because reading a
@StateObjectoutside a SwiftUI view creates a new object on every read.Related: #11095 is an open 2023 PR that took the same approach against the old
FirebaseRemoteConfigSwift/Sources/PropertyWrapperpath, which no longer exists.Testing
XCTAssertEqual failed: ("remote1") is not equal to ("default")and
XCTAssertEqual failed: ("remote2") is not equal to ("fallback").xcodebuild -scheme RemoteConfigFakeConsole test -destination 'platform=macOS': 49 tests, 0failures.
-only-testing:RemoteConfigFakeConsole/PropertyWrapperTests -test-iterations 5 -run-tests-until-failure: 20 test runs, 0 failures.Fixes #16816