[#639] Add local and remote config management with AppConfig [PART 1]#693
Open
thinh2k1310 wants to merge 4 commits into
Open
[#639] Add local and remote config management with AppConfig [PART 1]#693thinh2k1310 wants to merge 4 commits into
thinh2k1310 wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds a Firebase-backed remote configuration system: dynamic coding keys and default config encoding, a RemoteConfig decoder and Firebase adapter, a reactive AppConfig manager publishing decoded configs, DI wiring for Firebase and example AppConfig, and Tuist package/test dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AC as AppConfig
participant RC as FirebaseRemoteConfig
participant Dec as RemoteConfigDecoder
participant Sub as CurrentValueSubject
App->>AC: setUp()
activate AC
AC->>RC: fetchAndActivate() (async)
activate RC
RC-->>AC: fetch result / updated values
deactivate RC
AC->>Dec: init(remoteConfig)
activate Dec
Dec->>RC: allKeys / decodeValue(forKey:)
RC-->>Dec: raw entries
Dec-->>AC: DecodedConfig
deactivate Dec
AC->>Sub: send(decodedConfig)
deactivate AC
Note over Sub: Subscribers receive updated config via publisher
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
thinh2k1310
force-pushed
the
feature/#639-add-local-and-remote-config
branch
3 times, most recently
from
bcdbef9 to
9e224fb
Compare
thinh2k1310
changed the title
thinh2k1310
force-pushed
the
feature/#639-add-local-and-remote-config
branch
from
9e224fb to
b86cec1
Compare
thinh2k1310
requested review from
Shayokh144,
Thieurom,
donmartial,
dquangit,
ducbm051291,
kevinhoangpq,
markgravity,
minhnimble,
nhgia,
nkhanh44,
nmint8m,
patcharapon-j,
phongvhd93,
roman-nimble,
suho and
vnntsu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
template/Modules/Data/Sources/Extensions/Container+Data.swift (1)
18-20: Consider returning the protocol type for consistency.The
firebaseRemoteConfigSourcefactory returns the concrete typeFirebaseRemoteConfigSource, whileremoteConfigRepositoryreturns the protocol typeRemoteConfigRepository. For consistency and to facilitate test overrides at the container level, consider usingany RemoteConfigSource.♻️ Proposed fix
- public var firebaseRemoteConfigSource: Factory<FirebaseRemoteConfigSource> { + public var firebaseRemoteConfigSource: Factory<any RemoteConfigSource> { self { FirebaseRemoteConfigSource() }.singleton }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/Extensions/Container`+Data.swift around lines 18 - 20, Change the container factory to return the protocol type instead of the concrete implementation so callers and tests can depend on the abstraction: update the firebaseRemoteConfigSource Factory definition to produce any RemoteConfigSource (rather than FirebaseRemoteConfigSource) while still constructing FirebaseRemoteConfigSource() inside the factory and keeping .singleton; this keeps remoteConfigRepository and other consumers consistently typed to RemoteConfigSource and allows easy test overrides at the container level.template/Modules/Data/Sources/AppConfig/AppDefaultConfig.swift (1)
10-10: Consider makingconfigsimmutable.The
configsproperty is declared asvarbut isn't mutated after initialization. For a value-semanticEncodablestruct, preferletto signal immutability and prevent accidental mutation.♻️ Proposed fix
- public var configs: [AnyCodingKey: any Encodable] + public let configs: [AnyCodingKey: any Encodable]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/AppDefaultConfig.swift` at line 10, The configs property in AppDefaultConfig is declared as a mutable var but never mutated; change it to an immutable let to convey value semantics: replace "public var configs: [AnyCodingKey: any Encodable]" with "public let configs: [AnyCodingKey: any Encodable]" in the AppDefaultConfig definition (and update or add the initializer for AppDefaultConfig if necessary so configs is set at construction time).template/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swift (1)
57-67: Inconsistent nil handling between decode methods.
decodeBoolreturns a non-optionalBoolanddecodeNumberreturnsNSNumber?without checking for missing keys, whiledecodeDataanddecodeStringexplicitly returnnilfor empty values. Firebase'sboolValuedefaults tofalsefor missing keys, which could silently mask configuration errors.Consider making these methods consistent—either all return optionals with explicit nil for missing keys, or document the fallback behavior clearly.
♻️ Proposed fix for consistent nil handling
- public func decodeBool(forKey keyString: String) -> Bool { - let value = remoteConfig[keyString].boolValue - log("Decoded bool for \(keyString): \(value)") - return value + public func decodeBool(forKey keyString: String) -> Bool? { + let configValue = remoteConfig[keyString] + // Check if the value actually exists (non-empty data) + guard !configValue.dataValue.isEmpty else { + log("No bool value for key: \(keyString)") + return nil + } + let value = configValue.boolValue + log("Decoded bool for \(keyString): \(value)") + return value } - public func decodeNumber(forKey keyString: String) -> NSNumber? { - let value = remoteConfig[keyString].numberValue + public func decodeNumber(forKey keyString: String) -> NSNumber? { + let configValue = remoteConfig[keyString] + guard !configValue.dataValue.isEmpty else { + log("No number value for key: \(keyString)") + return nil + } + let value = configValue.numberValue log("Decoded number for \(keyString): \(value)") return value }Note: If you change
decodeBoolto returnBool?, you'll need to update the call site inExampleAppConfiguration.swiftto provide a fallback:isFeatureEnabled: decoder.decodeBool(forKey: ExampleConfigKey.isFeatureEnabled.rawValue) ?? false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swift` around lines 57 - 67, The decode methods have inconsistent nil handling: decodeData/decodeString return nil for empty/missing values while decodeBool returns non-optional (Firebase boolValue defaults to false) and decodeNumber returns an optional but doesn't explicitly handle missing keys; change decodeBool to return Bool? (and ensure decodeNumber explicitly returns nil for missing keys if it doesn't already) so all decoders consistently return optionals for absent values (refer to decodeBool, decodeNumber, decodeData, decodeString), update call sites such as ExampleAppConfiguration.swift to apply explicit fallbacks (e.g., decodeBool(...) ?? false) and keep the existing log calls to record decoded result or absence.template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift (1)
53-62: Simplify the guard conditions for clarity.The current guard logic on lines 56-62 is confusing:
- Line 56 passes if source is not
.staticOR data is not empty- Line 60 then checks if data is not empty again
The intent appears to be: return
nilfor empty data, OR for.staticsource with empty data. Consider consolidating for readability.♻️ Proposed simplification
public func value(forKey key: String) async -> RemoteConfigStoredValue? { let (data, source) = remoteConfig.configEntry(forKey: key) - guard source != .static || !data.isEmpty else { - return nil - } - - guard !data.isEmpty else { + // Return nil for empty data, or for static source entries (no remote/default value set) + guard !data.isEmpty, source != .static else { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift` around lines 53 - 62, The two guard checks in value(forKey:) are redundant; simplify by replacing both guards with a single check that returns nil when data.isEmpty. In the function using remoteConfig.configEntry(forKey:), remove the guard that tests source != .static || !data.isEmpty and the duplicate guard for !data.isEmpty, and keep one guard !data.isEmpty else { return nil } so you only bail out on empty data (referencing the value(forKey:) function, the remoteConfig.configEntry(forKey:) call, the source == .static case and data.isEmpty predicate).template/Modules/Data/Sources/AppConfig/AppConfig.swift (1)
32-36: Avoid exposingCurrentValueSubjectas public mutable state.External code can call
senddirectly and bypass the Remote Config update path. Make the subject private and expose only the publisher/current snapshot.Suggested change
- public let currentConfigSubject: CurrentValueSubject<DecodedConfig, Never> + private let currentConfigSubject: CurrentValueSubject<DecodedConfig, Never>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift` around lines 32 - 36, The CurrentValueSubject currentConfigSubject is publicly writable and should be made private to prevent external send() calls; change the declaration of currentConfigSubject to private (or private let) and keep the existing public currentConfigPublisher (currentConfigSubject.eraseToAnyPublisher()) for subscribers, and add a read-only public accessor (e.g., a currentConfig computed property that returns currentConfigSubject.value) so callers can read the latest DecodedConfig without being able to mutate it; update any internal usages to reference the now-private currentConfigSubject directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift`:
- Around line 60-62: getAllKeysFromDefault currently returns an empty array if
remoteConfig isn't initialized; change it to derive keys from the local default
configuration instead of depending on remoteConfig initialization. Update
getAllKeysFromDefault to return keys from your AppDefaultConfig (or whatever
struct/enum holds your bundled defaults) — e.g., compute and return
AppDefaultConfig.allKeys (or map the default dictionary) when remoteConfig is
nil — or alternatively add a clear precondition/comment on setUp() and
throw/return a Result indicating uninitialized state; reference
getAllKeysFromDefault, remoteConfig, AppDefaultConfig and setUp when making the
change.
- Around line 52-58: setUp() is currently not idempotent and setUpListener()
registers duplicate listeners on the RemoteConfig singleton; add an idempotence
guard (e.g. an isSetUp Bool or store the listener handle) so repeated calls to
setUp() or setUpListener() do not register additional observers. Specifically,
check a stored flag or existing listener handle before creating
remoteConfig/adding the listener and set that flag/handle when you first
register; ensure fetchAndActivate() and setUpDefaults()/setUpConfigSettings()
are still called only once or guarded, and apply the same idempotence fix to the
other setup block referenced (lines around the second setUpListener usage) so
publishUpdatedConfig() / currentConfigSubject.send() will not be invoked
multiple times per config update.
In `@template/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swift`:
- Around line 38-54: The default welcome message is inconsistent:
AppDefaultConfig.build uses "Welcome to {PROJECT_NAME}" while configMapper falls
back to "Welcome"; update the fallback in the configMapper closure (the mapping
for welcomeMessage inside the ExampleAppConfiguration initializer) to match the
default string used in AppDefaultConfig.build
(ExampleConfigKey.welcomeMessage.rawValue) so both AppDefaultConfig.build and
the RemoteConfigDecoder fallback return the same branded message.
---
Nitpick comments:
In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift`:
- Around line 32-36: The CurrentValueSubject currentConfigSubject is publicly
writable and should be made private to prevent external send() calls; change the
declaration of currentConfigSubject to private (or private let) and keep the
existing public currentConfigPublisher
(currentConfigSubject.eraseToAnyPublisher()) for subscribers, and add a
read-only public accessor (e.g., a currentConfig computed property that returns
currentConfigSubject.value) so callers can read the latest DecodedConfig without
being able to mutate it; update any internal usages to reference the now-private
currentConfigSubject directly.
In `@template/Modules/Data/Sources/AppConfig/AppDefaultConfig.swift`:
- Line 10: The configs property in AppDefaultConfig is declared as a mutable var
but never mutated; change it to an immutable let to convey value semantics:
replace "public var configs: [AnyCodingKey: any Encodable]" with "public let
configs: [AnyCodingKey: any Encodable]" in the AppDefaultConfig definition (and
update or add the initializer for AppDefaultConfig if necessary so configs is
set at construction time).
In `@template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift`:
- Around line 53-62: The two guard checks in value(forKey:) are redundant;
simplify by replacing both guards with a single check that returns nil when
data.isEmpty. In the function using remoteConfig.configEntry(forKey:), remove
the guard that tests source != .static || !data.isEmpty and the duplicate guard
for !data.isEmpty, and keep one guard !data.isEmpty else { return nil } so you
only bail out on empty data (referencing the value(forKey:) function, the
remoteConfig.configEntry(forKey:) call, the source == .static case and
data.isEmpty predicate).
In `@template/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swift`:
- Around line 57-67: The decode methods have inconsistent nil handling:
decodeData/decodeString return nil for empty/missing values while decodeBool
returns non-optional (Firebase boolValue defaults to false) and decodeNumber
returns an optional but doesn't explicitly handle missing keys; change
decodeBool to return Bool? (and ensure decodeNumber explicitly returns nil for
missing keys if it doesn't already) so all decoders consistently return
optionals for absent values (refer to decodeBool, decodeNumber, decodeData,
decodeString), update call sites such as ExampleAppConfiguration.swift to apply
explicit fallbacks (e.g., decodeBool(...) ?? false) and keep the existing log
calls to record decoded result or absence.
In `@template/Modules/Data/Sources/Extensions/Container`+Data.swift:
- Around line 18-20: Change the container factory to return the protocol type
instead of the concrete implementation so callers and tests can depend on the
abstraction: update the firebaseRemoteConfigSource Factory definition to produce
any RemoteConfigSource (rather than FirebaseRemoteConfigSource) while still
constructing FirebaseRemoteConfigSource() inside the factory and keeping
.singleton; this keeps remoteConfigRepository and other consumers consistently
typed to RemoteConfigSource and allows easy test overrides at the container
level.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fca0d3f-6ea4-4dd8-a5c9-0e0610000e37
📒 Files selected for processing (9)
template/Modules/Data/Sources/AppConfig/AnyCodingKey.swifttemplate/Modules/Data/Sources/AppConfig/AppConfig.swifttemplate/Modules/Data/Sources/AppConfig/AppDefaultConfig.swifttemplate/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swifttemplate/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swifttemplate/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swifttemplate/Modules/Data/Sources/Extensions/Container+Data.swifttemplate/Tuist/ProjectDescriptionHelpers/Module.swifttemplate/Tuist/ProjectDescriptionHelpers/Target+Initializing.swift
thinh2k1310
force-pushed
the
feature/#639-add-local-and-remote-config
branch
from
b86cec1 to
2c1c3ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
template/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swift (1)
16-20:⚠️ Potential issue | 🟡 MinorUse one source of truth for the example defaults.
welcomeMessagedefaults to"Welcome"ininitialConfigand decoder fallback, but the Remote Config default is"Welcome to {PROJECT_NAME}". Align them so initial/local/fallback behavior is consistent.🔧 Proposed fix
public init( isFeatureEnabled: Bool = false, maxRetryCount: Int = 3, apiTimeout: Double = 30.0, - welcomeMessage: String = "Welcome" + welcomeMessage: String = "Welcome to {PROJECT_NAME}" @@ - welcomeMessage: decoder.decodeString(forKey: ExampleConfigKey.welcomeMessage.rawValue) ?? "Welcome" + welcomeMessage: decoder.decodeString(forKey: ExampleConfigKey.welcomeMessage.rawValue) ?? "Welcome to {PROJECT_NAME}"Also applies to: 38-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swift` around lines 16 - 20, The example uses three different defaults for the welcome message; unify them by introducing a single constant (e.g., DEFAULT_WELCOME_MESSAGE) and use it everywhere: in the public init signature default for welcomeMessage, in initialConfig, and in the decoder fallback logic (init(from:) or decoding helper) so the Remote Config default, local initialConfig and decoder fallback all read "Welcome to {PROJECT_NAME}" consistently; update occurrences referenced by public init, initialConfig, and the decoder fallback method to use that constant.template/Modules/Data/Sources/AppConfig/AppConfig.swift (2)
60-62:⚠️ Potential issue | 🟡 MinorDerive default keys from local defaults instead of Remote Config state.
getAllKeysFromDefault()returns[]beforesetUp(), even thoughdefaultConfigis already available locally. This makes the “default” API depend on Firebase initialization.🔧 Proposed fix
public func getAllKeysFromDefault() -> [String] { - remoteConfig?.allKeys(from: .default) ?? [] + defaultConfig.configs.keys.map(\.stringValue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift` around lines 60 - 62, getAllKeysFromDefault() currently depends on remoteConfig being initialized so it returns [] before setUp(); change it to fall back to the local defaultConfig when remoteConfig is nil. Update the method (getAllKeysFromDefault) to return remoteConfig?.allKeys(from: .default).map(String.init) ?? Array(defaultConfig.keys.map { String($0) }) (or equivalent conversion) so it derives keys from the local defaultConfig property when remoteConfig is not available; keep using remoteConfig.allKeys(from: .default) when remoteConfig exists.
52-58:⚠️ Potential issue | 🟠 MajorMake setup/listener registration idempotent and removable.
setUp()can still register multiple Firebase update listeners on repeated calls, and the returned registration is discarded, so the listener cannot be removed. Store the registration, guard repeated setup, and remove it indeinit. Firebase’s Swift reference documents thataddOnConfigUpdateListenerreturns aConfigUpdateListenerRegistrationthat can remove the listener: https://firebase.google.com/docs/reference/swift/firebaseremoteconfig/api/reference/Classes/ConfigUpdateListenerRegistration🔁 Proposed fix
public final class AppConfig<DecodedConfig: Sendable>: AppConfigProtocol { private var remoteConfig: RemoteConfig? + private var configUpdateListenerRegistration: ConfigUpdateListenerRegistration? + private var isSetUp = false private let defaultConfig: AppDefaultConfig private let configMapper: (RemoteConfigDecoder) -> DecodedConfig @@ public init( defaultConfig: AppDefaultConfig = AppDefaultConfig(), initialConfig: DecodedConfig, configMapper: `@escaping` (RemoteConfigDecoder) -> DecodedConfig @@ currentConfigSubject = CurrentValueSubject(initialConfig) } + + deinit { + configUpdateListenerRegistration?.remove() + } public func setUp() { + guard !isSetUp else { return } + isSetUp = true remoteConfig = RemoteConfig.remoteConfig() setUpConfigSettings() setUpDefaults() @@ private func setUpListener() { - remoteConfig?.addOnConfigUpdateListener { [weak self] _, error in + configUpdateListenerRegistration = remoteConfig?.addOnConfigUpdateListener { [weak self] _, error in guard let self else { return } if logError(error, context: "Listener update") { return } remoteConfig?.activate { [weak self] _, error inFirebase iOS Swift RemoteConfig addOnConfigUpdateListener ConfigUpdateListenerRegistration remove official referenceAlso applies to: 91-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift` around lines 52 - 58, setUp() currently re-registers Firebase listeners on each call and discards the returned registration; make setup/listener registration idempotent by adding a stored optional property (e.g., var configUpdateRegistration: ConfigUpdateListenerRegistration?) and a Bool or guard in setUp()/setUpListener() to return early if already set up, call remoteConfig.addOnConfigUpdateListener and assign its return to configUpdateRegistration, and implement deinit to call configUpdateRegistration?.remove() so the listener can be removed; update related cleanup in setUpDefaults()/fetchAndActivate() if needed to rely on the single registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift`:
- Around line 32-40: Make the CurrentValueSubject backing store private to
prevent external callers from sending arbitrary values: change the visibility of
currentConfigSubject from public to private (e.g., private let
currentConfigSubject) while keeping the public read-only accessors
currentConfigPublisher and currentConfig unchanged; update any internal
initializers or methods in AppConfig that reference currentConfigSubject to use
the now-private property so the only external surface remains the read-only
publisher (currentConfigPublisher) and value (currentConfig).
In `@template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift`:
- Around line 64-78: The current string-to-typed conversion checks
normalizedRemoteConfigBoolean before numeric parsing, causing "0"/"1" to be
treated as Bool; change the ordering in the conversion block in
FirebaseRemoteConfigSource.swift to try Int and Double parsing before checking
normalizedRemoteConfigBoolean (i.e., attempt Int(string.trimmingCharacters(...))
then Double(...) then normalizedRemoteConfigBoolean), so numeric tokens flow to
.int/.double and only non-numeric boolean tokens map to .bool.
- Around line 56-62: The first guard condition (guard source != .static ||
!data.isEmpty) is redundant because the subsequent guard (!data.isEmpty) already
returns for empty payloads; collapse them by removing the first guard and
keeping only guard !data.isEmpty else { return nil } if the goal is simply to
bail on empty data, or if the real intent is to skip all .static entries change
the logic to guard source != .static else { return nil } so the code uses either
the empty-data check (data) or the source-only check (source != .static) but not
both redundant guards.
- Around line 13-39: The protocol RemoteConfigInterface must be marked Sendable
to satisfy RemoteConfigSource's Sendable conformance and avoid concurrency
warnings; update the protocol declaration (RemoteConfigInterface) to conform to
Sendable, ensure any types used in its requirements (e.g., return types like
FirebaseRemoteConfig.RemoteConfigSource and Data) are Sendable or bridged, and
then confirm the extension on RemoteConfig and the stored property remoteConfig
in FirebaseRemoteConfigSource remain valid (RemoteConfig and
FirebaseRemoteConfigSource should still compile with the new protocol
constraint).
---
Duplicate comments:
In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift`:
- Around line 60-62: getAllKeysFromDefault() currently depends on remoteConfig
being initialized so it returns [] before setUp(); change it to fall back to the
local defaultConfig when remoteConfig is nil. Update the method
(getAllKeysFromDefault) to return remoteConfig?.allKeys(from:
.default).map(String.init) ?? Array(defaultConfig.keys.map { String($0) }) (or
equivalent conversion) so it derives keys from the local defaultConfig property
when remoteConfig is not available; keep using remoteConfig.allKeys(from:
.default) when remoteConfig exists.
- Around line 52-58: setUp() currently re-registers Firebase listeners on each
call and discards the returned registration; make setup/listener registration
idempotent by adding a stored optional property (e.g., var
configUpdateRegistration: ConfigUpdateListenerRegistration?) and a Bool or guard
in setUp()/setUpListener() to return early if already set up, call
remoteConfig.addOnConfigUpdateListener and assign its return to
configUpdateRegistration, and implement deinit to call
configUpdateRegistration?.remove() so the listener can be removed; update
related cleanup in setUpDefaults()/fetchAndActivate() if needed to rely on the
single registration.
In `@template/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swift`:
- Around line 16-20: The example uses three different defaults for the welcome
message; unify them by introducing a single constant (e.g.,
DEFAULT_WELCOME_MESSAGE) and use it everywhere: in the public init signature
default for welcomeMessage, in initialConfig, and in the decoder fallback logic
(init(from:) or decoding helper) so the Remote Config default, local
initialConfig and decoder fallback all read "Welcome to {PROJECT_NAME}"
consistently; update occurrences referenced by public init, initialConfig, and
the decoder fallback method to use that constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dcf04bb-06d7-44ef-b953-1eef3fd9b983
📒 Files selected for processing (9)
template/Modules/Data/Sources/AppConfig/AnyCodingKey.swifttemplate/Modules/Data/Sources/AppConfig/AppConfig.swifttemplate/Modules/Data/Sources/AppConfig/AppDefaultConfig.swifttemplate/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swifttemplate/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swifttemplate/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swifttemplate/Modules/Data/Sources/Extensions/Container+Data.swifttemplate/Tuist/ProjectDescriptionHelpers/Module.swifttemplate/Tuist/ProjectDescriptionHelpers/Target+Initializing.swift
✅ Files skipped from review due to trivial changes (1)
- template/Modules/Data/Sources/AppConfig/AnyCodingKey.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- template/Tuist/ProjectDescriptionHelpers/Module.swift
- template/Modules/Data/Sources/Extensions/Container+Data.swift
- template/Modules/Data/Sources/AppConfig/AppDefaultConfig.swift
- template/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swift
thinh2k1310
added a commit
that referenced
this pull request
Apr 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift`:
- Around line 33-35: The app crashes because FirebaseRemoteConfigSource's
convenience init uses RemoteConfig.remoteConfig() as a default parameter which
runs before FirebaseApp.configure(); update startup to call
FirebaseApp.configure() before the DI container resolves remoteConfigRepository
(e.g., call FirebaseApp.configure() in App.swift or the app bootstrap before
Container+Application.swift resolves use cases), or change
FirebaseRemoteConfigSource to defer the RemoteConfig lookup by removing
RemoteConfig.remoteConfig() as a default and instead lazily obtaining
RemoteConfig (e.g., add an init(config: RemoteConfig?) and resolve
RemoteConfig.remoteConfig() only after FirebaseApp.configure() has run) so that
RemoteConfig.remoteConfig() is never invoked during DI resolution prior to app
bootstrap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47d76536-4f1f-4978-807c-ec7dbae262e4
📒 Files selected for processing (2)
template/Modules/Data/Sources/AppConfig/AppConfig.swifttemplate/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- template/Modules/Data/Sources/AppConfig/AppConfig.swift
ducbm051291
reviewed
Apr 23, 2026
thinh2k1310
added a commit
that referenced
this pull request
Apr 23, 2026
thinh2k1310
force-pushed
the
feature/#639-add-local-and-remote-config
branch
from
778a636 to
327c4a7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
template/Modules/Data/Sources/AppConfig/AppConfig.swift (1)
59-61:⚠️ Potential issue | 🟡 MinorReturn local default keys without requiring Remote Config setup.
getAllKeysFromDefault()currently returns[]beforesetUp(), even thoughdefaultConfigalready owns the local keys. This weakens the default/remote separation from the PR objective.Suggested change
public func getAllKeysFromDefault() -> [String] { - remoteConfig?.allKeys(from: .default) ?? [] + defaultConfig.configs.keys.map(\.stringValue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift` around lines 59 - 61, getAllKeysFromDefault currently relies on remoteConfig and returns an empty array before setUp(); change it to fall back to the local defaultConfig keys when remoteConfig is nil: if remoteConfig is present return remoteConfig!.allKeys(from: .default) as [String], otherwise extract and return the keys from defaultConfig (convert to [String] appropriately). Update the implementation of getAllKeysFromDefault to reference remoteConfig, defaultConfig, and setUp() behavior so local defaults are returned even before Remote Config is initialized.template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift (1)
33-35:⚠️ Potential issue | 🔴 CriticalKeep Firebase singleton resolution behind configured app bootstrap.
init()still callsRemoteConfig.remoteConfig()eagerly, so any DI resolution beforeFirebaseApp.configure()can still crash. If PART 2 owns this, please ensure the no-arg initializer is only used after bootstrap or remove it in favor of explicit injection.#!/bin/bash # Verify Firebase bootstrap happens before FirebaseRemoteConfigSource is resolved. # Expected: FirebaseApp.configure() appears in app bootstrap before any factory/use case resolves FirebaseRemoteConfigSource. echo "--- FirebaseApp.configure call sites ---" rg -nP -C3 'FirebaseApp\.configure\s*\(' echo echo "--- FirebaseRemoteConfigSource construction call sites ---" rg -nP -C3 '\bFirebaseRemoteConfigSource\s*\(' echo echo "--- Factory/DI registrations mentioning remote config ---" rg -nP -C3 '\b(firebaseRemoteConfigSource|remoteConfigRepository|RemoteConfigSource)\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift` around lines 33 - 35, The convenience init currently calls RemoteConfig.remoteConfig() eagerly which can crash if FirebaseApp.configure() hasn't run; remove the no-arg convenience initializer from FirebaseRemoteConfigSource (or at minimum replace it with a guarded init that checks FirebaseApp.app() != nil and fails/throws/assert with a clear message) and require callers to use the explicit init(config:) so DI/bootstrapping must provide RemoteConfig after FirebaseApp.configure(); update references/construction sites of FirebaseRemoteConfigSource and any DI factory registrations to pass an injected RemoteConfig instead of using the removed/changed convenience init.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@template/Modules/Data/Sources/AppConfig/AppConfig.swift`:
- Around line 59-61: getAllKeysFromDefault currently relies on remoteConfig and
returns an empty array before setUp(); change it to fall back to the local
defaultConfig keys when remoteConfig is nil: if remoteConfig is present return
remoteConfig!.allKeys(from: .default) as [String], otherwise extract and return
the keys from defaultConfig (convert to [String] appropriately). Update the
implementation of getAllKeysFromDefault to reference remoteConfig,
defaultConfig, and setUp() behavior so local defaults are returned even before
Remote Config is initialized.
In `@template/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swift`:
- Around line 33-35: The convenience init currently calls
RemoteConfig.remoteConfig() eagerly which can crash if FirebaseApp.configure()
hasn't run; remove the no-arg convenience initializer from
FirebaseRemoteConfigSource (or at minimum replace it with a guarded init that
checks FirebaseApp.app() != nil and fails/throws/assert with a clear message)
and require callers to use the explicit init(config:) so DI/bootstrapping must
provide RemoteConfig after FirebaseApp.configure(); update
references/construction sites of FirebaseRemoteConfigSource and any DI factory
registrations to pass an injected RemoteConfig instead of using the
removed/changed convenience init.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bad3f7d-3645-4ba0-abce-617503b87b66
📒 Files selected for processing (9)
template/Modules/Data/Sources/AppConfig/AnyCodingKey.swifttemplate/Modules/Data/Sources/AppConfig/AppConfig.swifttemplate/Modules/Data/Sources/AppConfig/AppDefaultConfig.swifttemplate/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swifttemplate/Modules/Data/Sources/AppConfig/FirebaseRemoteConfigSource.swifttemplate/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swifttemplate/Modules/Data/Sources/Extensions/Container+Data.swifttemplate/Tuist/ProjectDescriptionHelpers/Module.swifttemplate/Tuist/ProjectDescriptionHelpers/Target+Initializing.swift
✅ Files skipped from review due to trivial changes (4)
- template/Tuist/ProjectDescriptionHelpers/Module.swift
- template/Modules/Data/Sources/AppConfig/AnyCodingKey.swift
- template/Modules/Data/Sources/AppConfig/AppDefaultConfig.swift
- template/Modules/Data/Sources/AppConfig/ExampleAppConfiguration.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- template/Modules/Data/Sources/Extensions/Container+Data.swift
- template/Modules/Data/Sources/AppConfig/RemoteConfigDecoder.swift
ducbm051291
approved these changes
Apr 24, 2026
markgravity
reviewed
May 4, 2026
Comment on lines
+42
to
+43
| defaultConfig: AppDefaultConfig = AppDefaultConfig(), | ||
| initialConfig: DecodedConfig, |
Contributor
There was a problem hiding this comment.
What is the difference between defaultConfig and initialConfig? 🤔
Contributor
Author
There was a problem hiding this comment.
initialConfig is the starting value for currentConfigSubject. defaultConfig is what we pass to Firebase as Remote Config defaults before fetching the server values. Bts, defaultConfig is built from initialConfig ( e.g., in ExampleAppConfiguration ). I renamed: defaultConfig -> remoteConfigDefaults, initialConfig -> bootstrapDecodedConfig to make this clearer 🙏 .
| apiTimeout: decoder.decodeNumber(forKey: ExampleConfigKey.apiTimeout.rawValue)?.doubleValue ?? 30.0, | ||
| welcomeMessage: decoder.decodeString(forKey: ExampleConfigKey.welcomeMessage.rawValue) ?? "Welcome" | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
what do you think about creating a AppConfigurationCodable protocol so that we can use it on ExampleAppConfiguration
struct ExampleAppConfiguration: AppConfigurationCodable {
init(decoder: RemoteConfigDecoder) {
// our mapper functions
}
func encode() -> XXX // should return a type that we can use to set to RemoteConfig, eg RemoteConfig.setDefaults()
}
thinh2k1310
force-pushed
the
feature/#639-add-local-and-remote-config
branch
from
97349c3 to
88f96a6
Compare
thinh2k1310
force-pushed
the
feature/#639-add-local-and-remote-config
branch
from
88f96a6 to
95285b0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happened 👀
AppConfig<T>: Generic Firebase Remote Config managerAnyCodingKey: Flexible CodingKey for dynamic configuration keysAppDefaultConfig: Encodable wrapper for default configuration valuesRemoteConfigDecoder: Type-safe helper for decoding remote config valuesFirebaseRemoteConfigSource: Bridge to existing Domain RemoteConfigSource protocolExampleAppConfiguration: Complete usage exampleInsight 📝
N/A
Proof Of Work 📹
Will add in PART 2
Summary by CodeRabbit
New Features
Chores