Firebase remote config instance loader #1945
Firebase remote config instance loader #1945mobilebytesenseicommunity wants to merge 13 commits intoopenMF:developmentfrom
Conversation
…switching - Add GitLive Firebase Remote Config SDK dependency - Create ServerInstance and InstancesConfig data models - Implement FirebaseInstanceConfigLoader to fetch instance configurations - Add InstanceConfigManager to manage selected instance - Update BaseURL to use dynamic configuration instead of hardcoded values - Update KtorInterceptor to use dynamic tenant ID from selected instance - Add datastore support for storing selected instance preferences - Create InstanceSelectorScreen with gesture detection (5 taps) - Add InstanceSelectorViewModel to manage instance selection - Integrate gesture detector in MifosApp to show instance selector dialog This feature allows users to dynamically switch between multiple backend instances without rebuilding the app. Instance configurations are fetched from Firebase Remote Config using a single parameter "instances_config_v1" containing JSON with instance details. Users can trigger the instance selector by tapping 5 times anywhere on the screen.
- Add InstanceType enum (MAIN, INTERBANK) to ServerInstance model
- Update InstancesConfig with helper methods to filter by instance type
- Extend datastore to store both selectedInstance and selectedInterbankInstance
- Update InstanceConfigManager to handle both main and interbank instances
- Add DEFAULT_INTERBANK_INSTANCE with apis.flexcore.mx configuration
- Update BaseURL.interBankUrl to use dynamic configuration from InstanceConfigManager
- Update FirebaseInstanceConfigLoader with default instances for both types
- Redesign InstanceSelectorScreen to display main and interbank instances separately
- Update InstanceSelectorViewModel to handle selection of both instance types
- Users can now independently select main server and interbank server instances
Firebase Remote Config JSON format now supports:
{
"instances": [
{
"endpoint": "mifos-bank-2.mifos.community",
"protocol": "https://",
"path": "/fineract-provider/api/v1/",
"platformTenantId": "mifos-bank-2",
"label": "Production Main",
"type": "MAIN",
"isDefault": true
},
{
"endpoint": "apis.flexcore.mx",
"protocol": "https://",
"path": "/v1.0/vnext1/",
"platformTenantId": "mifos-bank-2",
"label": "Production Interbank",
"type": "INTERBANK",
"isDefault": true
}
]
}
📝 WalkthroughWalkthroughAdds a multi-platform server instance selection feature: gesture-triggered bottom sheet UI, ViewModel and persistence for selected main/interbank instances, Firebase remote-config loading of instance lists, runtime network wiring to use chosen instances, and related model, datastore, DI, build, and resource updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as MifosPayApp
participant Detector as GestureDetector
participant Screen as InstanceSelectorScreen
participant VM as InstanceSelectorViewModel
participant ConfigLoader as FirebaseInstanceConfigLoader
participant Preferences as UserPreferencesRepository
participant Network as InstanceConfigManager
User->>Detector: Five quick taps
Detector->>App: onGestureDetected
App->>Screen: show InstanceSelectorScreen
Screen->>VM: collect state & observe events
VM->>ConfigLoader: observeInstancesConfig()
ConfigLoader-->>VM: InstancesConfig (DataState)
VM->>Preferences: observe selectedInstance / selectedInterbankInstance
Preferences-->>VM: StateFlow<ServerInstance?>
VM->>VM: combine -> InstanceSelectorState
VM-->>Screen: render grouped list
User->>Screen: select instance + tap Update
Screen->>VM: handleAction(UpdateInstances)
VM->>Preferences: updateSelectedInstance(...)
Preferences-->>VM: DataState.Success
VM->>VM: emit DismissSheet
VM-->>Screen: Dismiss event
Screen->>App: onDismiss
App->>App: hide sheet
App->>Network: InstanceConfigManager now reports selected instance for network calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/GestureDetector.kt (1)
22-47: Consider build-type restrictions for this feature.The 5-tap gesture to access server instance switching is a development/debug feature that could be accidentally triggered by users or misused in production. Consider restricting this gesture to debug/staging builds only.
core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt (1)
92-92: Hardcoded hostapis.flexcore.mxmay conflict with dynamic instance configuration.Since this PR introduces dynamic server instance selection, having hardcoded hosts in
loggableHostsmay be inconsistent. Consider deriving this from the config manager or removing it if no longer needed.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt (2)
60-65: Hardcoded strings should use string resources for i18n support.The strings "Select Instance", "Close", "Loading instances...", "Main Server Instances", "Interbank Server Instances", and "Tenant:" should be defined as string resources for proper internationalization.
101-114: Consider adding a retry mechanism for the error state.When an error occurs, users have no way to retry loading instances. Adding a retry button would improve the user experience.
is InstanceSelectorUiState.Error -> { Column( modifier = Modifier .fillMaxSize() .padding(paddingValues), horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.Center, ) { Text( text = "Error: ${state.message}", color = MaterialTheme.colorScheme.error, ) + Spacer(modifier = Modifier.height(16.dp)) + Button(onClick = { viewModel.loadInstances() }) { + Text("Retry") + } } }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (1)
68-70: Redundant Loading state handling.The
DataState.Loadingbranch sets the UI state toLoading, but this is already set at line 50 before the suspend call. IffetchInstancesConfig()is a suspend function that returns a final result (Success/Error), it should never returnLoading. If it can returnLoading, consider whether this pattern is intended.If
fetchInstancesConfig()always returns a terminal state, this branch can be removed to simplify the code.core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (1)
35-61: Surface Remote Config failures and avoid duplicating default configsRight now any exception, blank JSON, or empty
instanceslist is treated asDataState.Success(DEFAULT_CONFIG), with the original failure completely hidden. This can make diagnosing misconfigured Remote Config or serialization issues difficult, and callers can’t distinguish “remote defaulted” from “locally defaulted”.Additionally, the default main/interbank instances here duplicate the defaults defined in
InstanceConfigManager. If one is updated and the other isn’t, you’ll get divergent behavior depending on the code path.Consider:
- Logging or otherwise surfacing the exception (and optionally using a
DataState.Errorthat still carriesDEFAULT_CONFIGas a fallback payload).- Centralizing the default instance definitions (e.g., via a shared
InstancesConfig.DEFAULT/factory or by reusingInstanceConfigManager’s defaults) so there is a single canonical source of truth.Also applies to: 71-95
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigManager.kt (1)
20-42: De-duplicate default instances and harden self-service URL composition
InstanceConfigManagerprovides a nice single point of access for instance-derived URLs and tenant IDs. Two refinement points:
The
DEFAULT_MAIN_INSTANCE/DEFAULT_INTERBANK_INSTANCEdefinitions here duplicate the defaults inFirebaseInstanceConfigLoader. Consider extracting a shared default definition (e.g., in the model layer) so both the loader and manager depend on the same constants and can’t drift independently.
getSelfServiceUrl()assumespathalready includes a trailing/. To be more robust against future Remote Config changes, you could base this onfullUrl(e.g.,getCurrentInstance().fullUrl + "self/") or normalisepathto ensure exactly one/between segments.Also applies to: 44-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.kt(2 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/GestureDetector.kt(1 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt(1 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt(1 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt(5 hunks)core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesDataSource.kt(5 hunks)core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepository.kt(3 hunks)core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepositoryImpl.kt(3 hunks)core/model/src/commonMain/kotlin/org/mifospay/core/model/instance/InstancesConfig.kt(1 hunks)core/model/src/commonMain/kotlin/org/mifospay/core/model/instance/ServerInstance.kt(1 hunks)core/network/build.gradle.kts(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigManager.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt(3 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/BaseURL.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/KtorInterceptor.kt(5 hunks)gradle/libs.versions.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt (1)
core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptTheme.kt (1)
KptTheme(55-69)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt (1)
InstanceSelectorScreen(48-117)
core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt (2)
core-base/network/src/commonMain/kotlin/org/mifos/corebase/network/KtorHttpClient.kt (1)
setupDefaultHttpClient(67-162)core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/KtorInterceptor.kt (2)
install(33-52)install(76-97)
🔇 Additional comments (15)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.kt (1)
48-69: LGTM!The ViewModel registration follows the existing pattern and correctly wires
InstanceSelectorViewModelinto the shared Koin module.core/network/build.gradle.kts (1)
41-41: LGTM!The Firebase Remote Config dependency is correctly added to the commonMain source set, consistent with other gitlive Firebase dependencies in the project.
core/model/src/commonMain/kotlin/org/mifospay/core/model/instance/ServerInstance.kt (1)
19-31: LGTM!The
ServerInstancedata class is well-structured with sensible defaults and a usefulfullUrlcomputed property.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt (1)
174-184: LGTM!The Dialog integration correctly manages the show/dismiss state and renders the InstanceSelectorScreen appropriately.
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt (2)
48-117: LGTM overall!The screen structure with state handling for Loading, Success, and Error states follows good practices. The UI composition is clean and well-organized.
174-219: LGTM!The
InstanceItemcomponent is well-designed with clear visual hierarchy showing label, URL, and tenant information alongside the selection radio button.core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt (1)
32-46: Unfortunately, I'm unable to access the repository to verify this concern directly. However, based on the code snippet provided and the nature of the concern, I can offer my assessment:The review comment raises a valid architectural question. The concern states that
InstanceConfigLoaderis registered as a singleton butInstanceConfigManageronly receivesuserPreferencesRepositoryas a dependency.Without being able to inspect the actual
InstanceConfigManagerclass definition, I cannot definitively confirm whether:
- The loader is intentionally managed separately (e.g., lazy-loaded or initialized internally)
- The loader is truly orphaned and should be injected
- The manager derives its config through
userPreferencesRepositoryrather than directly from the loaderGiven the constraints, I recommend:
For the developer to verify this locally:
- Check if
InstanceConfigManagerinternally instantiates or usesInstanceConfigLoader- Confirm whether
userPreferencesRepositoryprovides the configuration data that would otherwise come from the loader- If the loader is unused, consider either removing it or injecting it into
InstanceConfigManager
InstanceConfigLoaderis registered but its relationship toInstanceConfigManagerrequires verification. The singleton is declared butInstanceConfigManagerreceives onlyuserPreferencesRepository. Confirm whether the manager needsInstanceConfigLoaderas an explicit dependency or if its configuration needs are satisfied through the preferences repository.gradle/libs.versions.toml (1)
223-223: Library version verified.The
dev.gitlive:firebase-config:2.1.0is a valid published version available on Maven Central Repository. The library entry follows the existing pattern for gitlive dependencies and uses the correct version reference.core/model/src/commonMain/kotlin/org/mifospay/core/model/instance/InstancesConfig.kt (1)
14-29: Clean model implementation with appropriate filtering logic.The data class is well-designed with immutable properties and functional helper methods. Using
firstOrNullgracefully handles the case where no default exists.One minor consideration: if the remote config contains multiple instances marked as
isDefault=truefor the same type, only the first one will be returned. This is likely acceptable behavior, but consider adding validation or logging if strict single-default enforcement is needed.core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepository.kt (1)
36-53: Interface additions follow existing patterns consistently.The new properties and methods align well with the existing API surface. Using
StateFlow<ServerInstance?>for observable selections andDataState<Unit>return types for updates maintains consistency with other repository methods.core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepositoryImpl.kt (2)
76-88: StateFlow properties follow existing conventions.The new
selectedInstanceandselectedInterbankInstanceproperties are implemented consistently with other StateFlow properties in the class, using the same scope, initial value, and sharing strategy.
100-116: Update methods are consistent with existing patterns.The implementation follows the same try/catch pattern as other update methods in this repository. While catching generic
Exceptionis broad, this maintains consistency with the existing codebase style.core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/BaseURL.kt (1)
14-34: ---Breaking change from singleton to DI-injectable class—verify all usages before merging.
The transition from a Kotlin
objectto a constructor-injected class enables dynamic server configuration. The delegation toInstanceConfigManagercentralizes endpoint resolution logic appropriately.However, this is a breaking change requiring all call sites to be updated. Verification is needed to confirm:
- All
BaseURLusages have been migrated to dependency injection patterns- No singleton-style direct accesses (
BaseURL.url,BaseURL.selfServiceUrl, etc.) remain in the codebase- DI container is properly wired to provide
BaseURLinstancescore/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (1)
23-26: Interface and loader wiring look coherentThe
InstanceConfigLoaderabstraction andFirebaseInstanceConfigLoaderimplementation are straightforward, DI‑friendly, and keep the Remote Config setup localized. The one‑shotobserveInstancesConfig()wrapper aroundfetchInstancesConfig()is also consistent with the current “fetch on demand” behavior.Also applies to: 28-34, 64-69, 98-100
core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/KtorInterceptor.kt (1)
21-26: Dynamic tenant header integration viaInstanceConfigManagerlooks correctWiring
InstanceConfigManagerinto bothKtorInterceptorandKtorInterceptorReand usinggetPlatformTenantId()forHEADER_TENANTaligns the HTTP layer with the active instance configuration. The newConfig/ConfigRefields continue the existinglateinitpattern, so as long asconfigManageris always set when installing the plugins, this should behave as intended.Also applies to: 33-39, 54-57, 61-64, 66-69, 76-83, 99-102, 106-109
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt
Outdated
Show resolved
Hide resolved
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt
Outdated
Show resolved
Hide resolved
core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesDataSource.kt
Show resolved
Hide resolved
core/model/src/commonMain/kotlin/org/mifospay/core/model/instance/ServerInstance.kt
Show resolved
Hide resolved
core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt
Show resolved
Hide resolved
- Remove remoteConfigSettings import (not available in GitLive SDK) - Change settings configuration to use suspend function - Update getString() to use bracket notation with asString() - Move settings configuration inside fetchInstancesConfig() method This fixes compilation errors with the GitLive Firebase SDK.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (2)
27-32: Consider injecting dependencies for testability.The
remoteConfigis directly accessed fromFirebase.remoteConfigsingleton, making unit testing difficult. Consider injecting the remote config instance via constructor to allow mocking in tests.-class FirebaseInstanceConfigLoader : InstanceConfigLoader { - private val remoteConfig = Firebase.remoteConfig +class FirebaseInstanceConfigLoader( + private val remoteConfig: FirebaseRemoteConfig = Firebase.remoteConfig, +) : InstanceConfigLoader { private val json = Json { ignoreUnknownKeys = true isLenient = true }
95-97:observeInstancesConfigdoesn't actually observe changes.The method name suggests continuous observation, but it only emits once and completes. Firebase Remote Config supports real-time updates via
addOnConfigUpdateListener. If continuous observation isn't needed, consider renaming tofetchInstancesConfigAsFlow()to clarify behavior.If one-shot flow is intentional, rename for clarity:
- override fun observeInstancesConfig(): Flow<DataState<InstancesConfig>> = flow { + override fun fetchInstancesConfigAsFlow(): Flow<DataState<InstancesConfig>> = flow { emit(fetchInstancesConfig()) }Or implement true observation using
callbackFlowwith Remote Config's update listener if real-time updates are desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt(1 hunks)
🔇 Additional comments (3)
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (3)
1-21: LGTM!License header and imports are appropriate for the Firebase Remote Config functionality.
22-25: LGTM!Clean interface design with appropriate separation between one-shot fetch and reactive observation patterns.
34-61: LGTM!The default configuration provides sensible fallback values. The fetch interval of 1 hour is reasonable for Remote Config. Having hardcoded defaults ensures the app remains functional even if Remote Config fails.
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt (1)
84-88: Hardcoded credentials - already flagged in previous review.core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (2)
82-112: Error handling masks failures and settings are reconfigured on every fetch.As previously noted, this method has several issues:
- All errors return
DataState.Success(DEFAULT_CONFIG), making it impossible for callers to distinguish successful remote fetch from fallback due to errors- Exceptions are silently swallowed without logging
remoteConfig.settingsis called on every fetch instead of once at initialization
82-112: Add logging to enable debugging of config fetch failures.The catch block silently returns
DEFAULT_CONFIGwith no logging. In production, there would be no visibility into:
- Whether remote config fetch succeeded or failed
- Why it failed (network, parsing, Firebase SDK issues)
- That fallback config is being used
Consider adding error logging in the catch block:
} catch (e: Exception) { + // Log error for production debugging + println("Failed to fetch remote config, using defaults: ${e.message}") // Return default config on error DataState.Success(DEFAULT_CONFIG) }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (1)
116-140: Persistence failures are unhandled, leading to data inconsistency.As previously noted, the results of
updateSelectedInstanceandupdateSelectedInterbankInstanceare discarded. If persistence fails:
- Temp selections are still reset (lines 135-136)
- Sheet is still dismissed (line 138)
- User believes their selection was saved, but it wasn't
On app restart, the previous selection would reappear, creating a confusing UX.
🧹 Nitpick comments (8)
core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/KtorInterceptor.kt (1)
64-100: Consider extracting common interceptor logic to reduce duplication.Both
KtorInterceptorandKtorInterceptorRehave nearly identicalinstall()implementations for setting headers and handling unauthorized responses. The only difference is how the token is obtained.Consider a shared helper or base implementation:
private fun HttpClient.installCommonHeaders(configManager: InstanceConfigManager) { requestPipeline.intercept(HttpRequestPipeline.State) { context.header(BaseURL.HEADER_CONTENT_TYPE, BaseURL.HEADER_CONTENT_TYPE_VALUE) context.header(BaseURL.HEADER_ACCEPT, BaseURL.HEADER_ACCEPT_VALUE) context.header(BaseURL.HEADER_TENANT, configManager.getPlatformTenantId()) } } private fun HttpClient.installUnauthorizedHandler() { responsePipeline.intercept(HttpResponsePipeline.After) { if (context.response.status == HttpStatusCode.Unauthorized) { GlobalAuthManager.markUnauthorized() } proceedWith(subject) } }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (2)
54-55: Inconsistent state declaration patterns.Line 54 uses
var showErrorDialog = remember { mutableStateOf<Boolean>(false) }requiring.valueaccess, while line 55 usesvar showInstanceSelector by remember { mutableStateOf(false) }with delegate syntax. For consistency, both should use the same pattern.- var showErrorDialog = remember { mutableStateOf<Boolean>(false) } + var showErrorDialog by remember { mutableStateOf(false) }Then update references from
showErrorDialog.valueto justshowErrorDialog.
93-100: Redundant MifosTheme wrapper.
InstanceSelectorScreenis already wrapped inMifosThemehere (lines 95-99), but the screen will be displayed while the mainMifosThemeat line 102 is also active. This nested theming is likely unnecessary and could cause subtle styling inconsistencies.// Show instance selector dialog if (showInstanceSelector) { - MifosTheme { - InstanceSelectorScreen( - onDismiss = { showInstanceSelector = false }, - ) - } + InstanceSelectorScreen( + onDismiss = { showInstanceSelector = false }, + ) }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt (2)
77-82: Modifier applied to both parent and child may cause unintended layout behavior.The same
modifieris passed to bothMifosBottomSheet(line 79) andMifosScaffold(line 82). This can lead to doubled padding, sizing constraints, or other layout issues depending on what modifiers are applied externally.Typically, the outer container should receive the external modifier:
MifosBottomSheet( onDismiss = onDismiss, modifier = modifier, ) { MifosScaffold( - modifier = modifier, + modifier = Modifier, bottomBar = {
337-355: Inconsistent theme usage in previews.
InstanceItemPreviewusesKptThemewhileInstanceItemUnselectedPreviewandInstancesListPreviewuseMifosTheme. Previews should use consistent theming to accurately represent how components will appear in the app.@Preview @Composable private fun InstanceItemPreview() { - KptTheme { + MifosTheme { InstanceItem(Also applies to: 357-375
core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/BaseURL.kt (1)
14-16: Add KDoc to document the class purpose and usage.Since
BaseURLis a public class and represents a significant refactor from a singleton to a DI-friendly component, consider adding KDoc to explain its role in providing runtime-configurable base URLs for network requests.core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (1)
114-116: Method name suggests continuous observation but implementation fetches once.The name
observeInstancesConfig()implies the Flow will emit updates when remote config changes, but it only fetches once and completes. Consider either:
- Renaming to
fetchInstancesConfigAsFlow()to clarify the one-shot behavior, or- Implementing true observation of Remote Config updates if Firebase supports listeners
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (1)
165-169: Remove unusedConfigLoadedaction.The
ConfigLoadedaction is defined but never dispatched. Config loading is handled directly in theinitblock viacombine(lines 41-42), not through the action handling system. The case at lines 110-112 is effectively dead code.sealed interface InstanceSelectorAction { data class SelectInstance(val instance: ServerInstance) : InstanceSelectorAction data object UpdateInstances : InstanceSelectorAction - data class ConfigLoaded(val config: DataState<InstancesConfig>) : InstanceSelectorAction }And remove the no-op handler:
is InstanceSelectorAction.UpdateInstances -> { updateInstances() } - - is InstanceSelectorAction.ConfigLoaded -> { - // Handled by init block - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmp-android/dependencies/prodReleaseRuntimeClasspath.txt(5 hunks)cmp-android/prodRelease-badging.txt(1 hunks)cmp-shared/src/commonMain/composeResources/values/strings.xml(1 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt(4 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt(1 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt(3 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/BaseURL.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/KtorInterceptor.kt(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt (1)
core-base/network/src/commonMain/kotlin/org/mifos/corebase/network/KtorHttpClient.kt (1)
setupDefaultHttpClient(67-162)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
🔇 Additional comments (10)
cmp-android/prodRelease-badging.txt (1)
1-131: I encountered a repository clone failure that prevents me from verifying the concerns raised in the review comment. Without access to the codebase, I cannot:
- Confirm whether
prodRelease-badging.txtis tracked in git or covered by.gitignore- Search for the mentioned classes (
InstanceSelectorViewModel,ServerInstance,InstancesConfig,InstanceConfigManager) to verify if source code files exist- Determine whether the actual source files implementing these features are part of this PR or missing
Since the automated verification is blocked and the review raises substantive claims about:
- Whether a generated artifact should be in version control
- Whether critical source code files are missing from the review
I must mark this for manual verification.
Auto-generated artifact may need version control review.
The
prodRelease-badging.txtfile is generated by Android's aapt tool. Verify whether this artifact should be committed to the repository or added to.gitignore. Generated artifacts can create merge conflicts and make diffs harder to review.Recommendation: Confirm the repository's policy on committing build artifacts like badging files, and adjust
.gitignoreif necessary.
Verify completeness of source code files for review.
The PR description mentions feature additions (InstanceSelectorViewModel, ServerInstance, InstancesConfig models, and InstanceConfigManager), but only a build artifact was provided for review. Confirm that all relevant source code files implementing these features are included and accessible for review.
cmp-android/dependencies/prodReleaseRuntimeClasspath.txt (3)
1-462: Verify classpath file generation and build consistency.This file appears to be auto-generated from a Gradle build process. Before merging:
- Confirm whether this file should be committed to version control or if it's regenerated during each build
- Run
./gradlew clean buildand verify the classpath file regenerates consistently- Ensure the PR contributor has completed the full test suite (
./gradlew check) as noted in the incomplete checklistThe PR description indicates the contributor checklist is incomplete (unit tests and style validation have not been checked).
456-462: Verify Kotlin 2.2.0 requirement for kotlinx-serialization 1.9.0 upgrade.The kotlinx-serialization 1.9.0 is a stable release, but it introduces a breaking change: it requires Kotlin 2.2.0 (the 1.8.1 version used Kotlin 2.1.x). Before merging this PR:
- Confirm the Kotlin Gradle plugin and compiler have been upgraded to 2.2.0 in this PR
- If kotlinx-datetime is used in the codebase, verify compatibility with v0.7.0+ (which removed kotlinx.datetime.Instant in favor of kotlin.time.Instant)
- Verify serialization for
ServerInstanceandInstancesConfigmodels works after the Kotlin upgrade- Run
./gradlew checkto test the upgrade (the PR checklist indicates tests have not yet been executed)
248-248: Based on my verification through web search, I can now provide an updated review comment:
Verify Compose compiler plugin compatibility with Kotlin 2.2.20 upgrade and confirm tests pass.
The Firebase dependency versions are stable and compatible:
dev.gitlive:firebase-config:2.4.0correctly works withfirebase-config:22.1.2(GitLive wrapper forwards to native Firebase SDKs)firebase-abt:21.1.1aligns with the Firebase Remote Config ecosystemkotlinx-serialization:1.9.0is compatible withkotlin-stdlib:2.2.20(both target Kotlin 2.2.x)- No public security advisories found for these versions
However, the Kotlin stdlib bump from 2.1.21 to 2.2.20 requires attention:
- Jetpack Compose UI 1.8.3 predates Kotlin 2.2.20; compatibility depends on using a matching Compose Compiler Gradle plugin version
- Ensure
composeCompilerVersionor equivalent is properly configured for Kotlin 2.2.20- The PR description shows unchecked "Run unit tests with ./gradlew check" — verify the full test suite passes, particularly for Compose-related code
Also applies to: 257-257, 301-306, 439-439, 456-462
core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/KtorInterceptor.kt (1)
23-56: LGTM! Config-driven header construction is a good improvement.The refactor to use
InstanceConfigManagerfor tenant resolution andBaseURLconstants for header names improves configurability and reduces hardcoded values.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
108-115: Gesture detection condition may not update reactively during navigation.The
navDestinationis computed once fromuiStateand won't react to actual navigation changes within the graph. If the user navigates from login to another screen without authentication state changes, the gesture will still be active.Consider using
navController.currentBackStackEntryAsState()for reactive navigation state:val currentEntry by navController.currentBackStackEntryAsState() val isOnLoginGraph = currentEntry?.destination?.parent?.route == LOGIN_GRAPH // ... modifier = modifier.detectInstanceSelectorGesture( onGestureDetected = { if (isOnLoginGraph) { showInstanceSelector = true } }, ),core/network/src/commonMain/kotlin/org/mifospay/core/network/di/NetworkModule.kt (2)
51-73: LGTM! Config-driven URL and header construction.The SelfClient now properly sources its base URL from
BaseURLand usesInstanceConfigManagerfor dynamic tenant configuration.
36-45: Based on my verification attempts, I was unable to access the repository due to network/access issues, which prevents me from confirming the actualInstanceConfigManagerconstructor signature and verifying whetherFirebaseInstanceConfigLoaderis used elsewhere in the codebase.However, the original review comment's logic is sound: if
FirebaseInstanceConfigLoaderis instantiated as a singleton but never injected or referenced, it would indicate an incomplete wiring issue in the dependency injection setup. The concern that Firebase config loading might not actually be wired into the manager is a legitimate one that requires manual verification by examining the actual class definitions and their usage patterns.
InstanceConfigLoadermay be instantiated but unused in the DI container.The review identifies a potential dependency injection wiring issue where
FirebaseInstanceConfigLoaderis created butInstanceConfigManagerdoesn't receive it. Verify that:
InstanceConfigManager's constructor actually expects aconfigLoaderparameter- The Firebase instance loading is intentionally delegated to
userPreferencesRepositoryIf
InstanceConfigManagershould use the loader, apply the suggested fix. Otherwise, remove the unusedFirebaseInstanceConfigLoaderdeclaration.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt (1)
229-335: Well-structured instance item component.Good implementation with clear visual hierarchy: label, type badge, default indicator, endpoint, and tenant ID. The clickable card with radio button provides good UX for selection.
core/network/src/commonMain/kotlin/org/mifospay/core/network/utils/BaseURL.kt (1)
27-37: I'll verify the performance concern about repeated property access in the BaseURL class. Let me start by searching for where these properties are accessed.
<function_calls>
#!/bin/bashFind all usages of BaseURL properties to assess access frequency
First, find the BaseURL class location and imports
rg -nP 'class BaseURL' --type=kotlin
Search for accesses to BaseURL properties
rg -nP '.(url|selfServiceUrl|interBankUrl|fineractPlatformTenantId)\b'
--type=kotlin
-C2
-g '!/test/'
</function_calls>Now let me examine the configManager implementation to understand what these methods do:
<function_calls>
#!/bin/bashFind configManager implementation and interface
rg -nP '(interface|class)\s+.*[Cc]onfigManager' --type=kotlin -A5
Search for getUrl, getSelfServiceUrl, etc. implementations
rg -nP 'fun\s+(getUrl|getSelfServiceUrl|getInterbankUrl|getPlatformTenantId)\s*('
--type=kotlin
-A5
</function_calls>Let me also check if there are any network interceptors or hot paths using these properties:
<function_calls>
#!/bin/bashSearch for network interceptors and their usage patterns
rg -nP '(Interceptor|intercept|OkHttp)' --type=kotlin -C2 | head -100
Look for BaseURL instantiation and injection patterns
rg -nP 'BaseURL' --type=kotlin -C1 | head -100
</function_calls>
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt
Show resolved
Hide resolved
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt
Outdated
Show resolved
Hide resolved
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt
Show resolved
Hide resolved
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (2)
38-88: Simplifycombine+stateIn+launchInpipelineYou create a
StateFlowviastateInand then immediatelylaunchIn(viewModelScope)on it, discarding both theStateFlowand theJob. The side‑effects already happen inside thecombineblock, so thestateInlayer is unused overhead.You can simplify to a single cold‑flow collection:
- combine( + combine( instanceConfigLoader.observeInstancesConfig(), userPreferencesRepository.selectedInstance, userPreferencesRepository.selectedInterbankInstance, tempSelectedMainInstance, tempSelectedInterbankInstance, ) { remoteConfig, selectedMainInstance, selectedInterbankInstance, tempMainInstance, tempInterbankInstance -> // existing when(remoteConfig) { ... } body - }.stateIn( - scope = viewModelScope, - started = SharingStarted.WhileSubscribed(5_000), - initialValue = null, - ).launchIn(viewModelScope) + }.launchIn(viewModelScope)This keeps the same behavior with less complexity and no unused
StateFlow.
115-138: Handle failures when persisting selected instances
updateInstancesalways dismisses the sheet and clears temps even if the underlying persistence calls fail (either by throwing or by returning an error result). That can leave the user believing the selection was saved when it wasn’t.At minimum, wrap the updates in
runCatching/try–catch, only dismiss on success, and surface an error on failure:private fun updateInstances() { viewModelScope.launch { - val currentState = state - val mainInstanceChanged = - tempSelectedMainInstance.value != currentState.selectedMainInstance - val interbankInstanceChanged = - tempSelectedInterbankInstance.value != currentState.selectedInterbankInstance - - if (mainInstanceChanged && tempSelectedMainInstance.value != null) { - userPreferencesRepository.updateSelectedInstance(tempSelectedMainInstance.value!!) - } - - if (interbankInstanceChanged && tempSelectedInterbankInstance.value != null) { - userPreferencesRepository.updateSelectedInterbankInstance( - tempSelectedInterbankInstance.value!!, - ) - } - - // Reset temp selections after update - tempSelectedMainInstance.value = null - tempSelectedInterbankInstance.value = null - - sendEvent(InstanceSelectorEvent.DismissSheet) + val currentState = state + val mainInstance = tempSelectedMainInstance.value + val interbankInstance = tempSelectedInterbankInstance.value + + val mainInstanceChanged = mainInstance != currentState.selectedMainInstance + val interbankInstanceChanged = interbankInstance != currentState.selectedInterbankInstance + + runCatching { + if (mainInstanceChanged && mainInstance != null) { + userPreferencesRepository.updateSelectedInstance(mainInstance) + } + if (interbankInstanceChanged && interbankInstance != null) { + userPreferencesRepository.updateSelectedInterbankInstance(interbankInstance) + } + }.onSuccess { + tempSelectedMainInstance.value = null + tempSelectedInterbankInstance.value = null + sendEvent(InstanceSelectorEvent.DismissSheet) + }.onFailure { + // Optionally log and reflect error in state for UI feedback + mutableStateFlow.update { it.copy(error = "Failed to update instances") } + } } }Adjust the error reporting to match your UX, but some form of failure handling here will make the flow more robust.
🧹 Nitpick comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (1)
163-167: Consider removing unusedConfigLoadedaction
InstanceSelectorAction.ConfigLoadedis defined but effectively a no‑op inhandleAction, and the configuration is already fully driven from theinitcombineblock.If there are no actual call sites sending
ConfigLoaded, consider dropping this variant to keep the action surface minimal and avoid confusion:sealed interface InstanceSelectorAction { data class SelectInstance(val instance: ServerInstance) : InstanceSelectorAction data object UpdateInstances : InstanceSelectorAction - data class ConfigLoaded(val config: DataState<InstancesConfig>) : InstanceSelectorAction }If you plan to use it later, a short KDoc comment explaining its intended role would also help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmp-android/prodRelease-badging.txt(1 hunks)cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
🔇 Additional comments (1)
cmp-android/prodRelease-badging.txt (1)
1-1: Version bump looks fineThe change here is just the
versionNamemetadata update; no behavioral impact.
…019GTifqCMdZeH5hUVeB4WKX
…r-019GTifqCMdZeH5hUVeB4WKX' into claude/firebase-remote-config-loader-019GTifqCMdZeH5hUVeB4WKX
therajanmaurya
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/GestureDetector.kt (1)
33-49: LGTM! Gesture detection logic is correct.The tap detection logic properly handles the timeout window and tap count. The implementation correctly resets the count when taps are too far apart and triggers the callback after 5 consecutive taps within 1 second.
Optional enhancement: Consider resetting
lastTapTimewhen the gesture is detected (line 45) for state consistency:if (tapCount >= 5) { onGestureDetected() tapCount = 0 + lastTapTime = 0L }This is not strictly necessary since the logic works correctly as-is, but it makes the state reset more complete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/GestureDetector.kt(1 hunks)
🔇 Additional comments (3)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/GestureDetector.kt (3)
1-9: LGTM!Standard copyright header with appropriate Mozilla Public License v2.0.
25-31: LGTM!Function signature and state initialization are well-structured. The 1-second timeout window for consecutive taps is a reasonable choice for gesture detection.
22-24: Now I have crucial context: the project uses Kotlin 1.9.x, which is correct for the timeline mentioned in the review comment. However, my earlier web search revealed an important nuance:Even in Kotlin 1.9+, the
@OptIn(ExperimentalTime::class)annotation is still needed if the code uses anykotlin.timemembers that remain experimental/preview. The annotation is not universally unnecessary just because Kotlin 1.9 stabilized parts of the API—it depends on which specific members are being used.Since I cannot clone the repository to inspect which
kotlin.timesymbols are actually being imported and used inGestureDetector.kt, I cannot definitively verify whether the annotation can be safely removed.
The
@OptIn(ExperimentalTime::class)annotation may still be necessary.While Kotlin 1.9+ stabilized most of the
kotlin.timeAPI, theExperimentalTimeopt-in is still required for any remaining experimental members. Verify which specifickotlin.timesymbols are imported and used in this file—if only stable APIs are used, the annotation can be removed; otherwise, it should be retained.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (2)
43-82: Unresolved:apis.flexcore.mxendpoint issue persists.This was flagged in a previous review. The endpoint returns HTTP 404 and has no public documentation. Please address the prior feedback before merging.
111-114: Unresolved: Silent error handling returns success state.This was flagged in a previous review. The catch block returns
DataState.Success(DEFAULT_CONFIG), masking failures from callers. Please address the prior feedback.
🧹 Nitpick comments (3)
core/common/src/commonMain/kotlin/org/mifospay/core/common/DeviceInfoProvider.kt (1)
26-31: Consider logging the exception before falling back.Silently swallowing all exceptions and returning
"unknown"can make debugging difficult. Consider logging the error at a debug/warning level to aid troubleshooting when Firebase Installation ID retrieval fails.core/common/src/nativeMain/kotlin/org/mifospay/core/common/BuildConfig.native.kt (1)
25-29: Track TODO: Retrieve version info from iOS bundle.The hardcoded version values are noted with TODOs. Consider creating a tracking issue to implement proper version retrieval from the iOS bundle (e.g.,
CFBundleShortVersionStringandCFBundleVersion).Would you like me to open an issue to track implementing proper iOS version retrieval?
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (1)
117-119: Method name implies continuous observation but only emits once.
observeInstancesConfigsuggests reactive updates when config changes, but this implementation performs a single fetch and emits. Consider either:
- Renaming to
fetchInstancesConfigAsFlow()to match behavior- Implementing actual observation using Firebase Remote Config listeners for real-time updates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/common/src/androidMain/kotlin/org/mifospay/core/common/BuildConfig.android.kt(1 hunks)core/common/src/androidMain/kotlin/org/mifospay/core/common/DeviceInfoProvider.android.kt(1 hunks)core/common/src/commonMain/kotlin/org/mifospay/core/common/BuildConfig.kt(1 hunks)core/common/src/commonMain/kotlin/org/mifospay/core/common/DeviceInfoProvider.kt(1 hunks)core/common/src/nativeMain/kotlin/org/mifospay/core/common/BuildConfig.native.kt(1 hunks)core/model/src/commonMain/kotlin/org/mifospay/core/model/instance/InstanceSelectorAccessControl.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/config/FeatureAccessManager.kt(1 hunks)core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt(1 hunks)
🔇 Additional comments (9)
core/model/src/commonMain/kotlin/org/mifospay/core/model/instance/InstanceSelectorAccessControl.kt (1)
14-19: LGTM!Clean and well-structured data class with sensible defaults for access control configuration.
core/common/src/commonMain/kotlin/org/mifospay/core/common/BuildConfig.kt (1)
15-37: LGTM!Clean interface design and well-organized constants for build configuration abstraction across platforms.
core/common/src/commonMain/kotlin/org/mifospay/core/common/DeviceInfoProvider.kt (1)
48-53: I'm experiencing persistent repository cloning issues that prevent me from executing the verification scripts. However, based on the review comment's concern about verifyingactualimplementations forexpectfunctions, I can provide guidance:The original review comment correctly identifies an important requirement for Kotlin Multiplatform code. The
expectfunctions declared incommonMainmust have correspondingactualimplementations in each platform source set.Given the technical limitations preventing direct verification, here is the rewritten review comment:
Verify that
actualimplementations exist for all platform targets.These
expectfunction declarations require correspondingactualimplementations in platform-specific source sets (androidMain, iosMain, nativeMain, etc.). Compilation will fail for any platform target missing these implementations. Ensure implementations are provided for all supported platforms.core/common/src/nativeMain/kotlin/org/mifospay/core/common/BuildConfig.native.kt (1)
35-37: Ensureactual object Platformimplementations exist for all native target platforms in this project.The
expect object Platformrequires matchingactualimplementations in each native target source set (e.g.,iosMain,macosMain) that the project targets. Per Kotlin Multiplatform standards, everyexpectdeclaration without@OptionalExpectationmust have anactualcounterpart for each compiled platform. Verify thatactualimplementations are provided in the appropriate platform-specific source sets to avoid compiler errors.core/common/src/androidMain/kotlin/org/mifospay/core/common/BuildConfig.android.kt (1)
15-30: LGTM!Clean implementation delegating to Android's generated
BuildConfig. Using getter properties ensures values are read at access time, which is appropriate for these compile-time constants.core/network/src/commonMain/kotlin/org/mifospay/core/network/config/FeatureAccessManager.kt (3)
36-49: Access control logic is sound.The precedence (debug → demo → remote config) is correctly implemented. Minor observation:
isDebugandbuildType == BuildType.DEBUGmay overlap depending on how the interface is implemented, but the defensive check is acceptable.
81-108: Allowlist logic is correct.The access control follows proper allowlist semantics: when both lists are empty, no restrictions apply; when either list has entries, the device or user must match to gain access.
111-119: LGTM!Well-structured data class for encapsulating device information. The nullable
userIdappropriately handles the case when users aren't logged in.core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt (1)
25-29: LGTM!Clean interface definition with appropriate async return types for remote config operations.
core/common/src/androidMain/kotlin/org/mifospay/core/common/DeviceInfoProvider.android.kt
Outdated
Show resolved
Hide resolved
core/common/src/commonMain/kotlin/org/mifospay/core/common/DeviceInfoProvider.kt
Outdated
Show resolved
Hide resolved
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigLoader.kt
Outdated
Show resolved
Hide resolved
therajanmaurya
force-pushed
the
claude/firebase-remote-config-loader-019GTifqCMdZeH5hUVeB4WKX
branch
from
58cf6e4 to
7ba0e26
Compare
…019GTifqCMdZeH5hUVeB4WKX
- Upgrade Kotlin to 2.2.21 for gitLive 2.4.0 compatibility - Update Compose Multiplatform to 1.9.3 - Update Ktor to 3.3.3 and Ktorfit to 2.7.1 - Update Firebase BOM to 34.7.0 - Update Android Gradle Plugin to 8.12.3 - Update Room to 2.8.4 - Add Firebase pods to iOS Podfile for gitlive support Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@cmp-ios/Podfile`:
- Around line 12-14: Remove the unused FirebasePerformance pod entry from the
Podfile: delete the line "pod 'FirebasePerformance'" and keep "pod
'FirebaseAnalytics'" and "pod 'FirebaseCrashlytics'"; after removing, run pod
install (or bundle exec pod install) to update the lockfile and ensure there are
no residual references to FirebasePerformance in the iOS build artifacts.
- Around line 9-14: The Podfile currently declares five Firebase pods without
version constraints and likely includes unnecessary pods for this PR; update the
Podfile to (1) remove pod 'FirebaseAnalytics', pod 'FirebaseCrashlytics', and
pod 'FirebasePerformance' unless gitlive/documentation or your app features
require them, keeping only pod 'FirebaseCore' and pod 'FirebaseRemoteConfig' for
Remote Config support, and (2) pin the remaining Firebase pods with a
pessimistic constraint (use ~> to the current major version used in the project)
so both pod 'FirebaseCore' and pod 'FirebaseRemoteConfig' include the same ~>
major.minor constraint to ensure reproducible installs and consistent versions.
In `@gradle/libs.versions.toml`:
- Around line 6-7: The listed versions in libs.versions.toml are invalid and
will cause Gradle sync failures; update androidGradlePlugin and androidTools to
actual published AGP/tool versions (replace androidGradlePlugin = "8.12.3" and
androidTools = "31.12.3" with a real release such as a published 8.1.x / 8.2.x /
9.0.x for androidGradlePlugin and the matching published androidTools
coordinate), verify the chosen versions exist on Maven Central, and run a Gradle
sync to confirm compatibility (look for androidGradlePlugin and androidTools in
the file to make the changes).
- Around line 349-353: Two aliases in gradle/libs.versions.toml point to the
same module: filekit-compose and filekit-dialogs-compose both map to
io.github.vinceglb:filekit-dialogs-compose; update code to use the canonical
alias and remove the duplicate. Replace all usages of libs.filekit.compose with
libs.filekit.dialogs.compose in the build.gradle.kts that currently imports
filekit (where libs.filekit.compose appears), then delete the redundant
filekit-compose entry from gradle/libs.versions.toml so only
filekit-dialogs-compose remains.
🧹 Nitpick comments (2)
gradle/libs.versions.toml (2)
87-93: Kotlin 2.3.0 is now available — consider upgrading.For the language release 2.2.0, there was only one tooling release 2.2.20 and one bugfix release 2.2.21. This confirms 2.2.21 is valid. However, Kotlin 2.3.0 has been released (Feb 5, 2026). If merging soon, you may want to bump
kotlinto2.3.0andkspto a compatible version to stay current. Note that Kotlin 2.3.0 brings Gradle 9.0 compatibility, which may be relevant if you plan to migrate to AGP 9.0.
1-134: Consider splitting the mass dependency upgrade from the feature PR.This version catalog update touches ~50+ dependency versions (AGP, Kotlin, Ktor, Compose, Firebase, Koin, Room, testing libraries, etc.), but the PR is titled "Firebase remote config instance loader." Bundling sweeping upgrades with a feature change makes it difficult to bisect regressions and complicates the review.
Ideally, the broad version bumps should be a separate, dedicated PR, and this PR should only add the dependencies directly required for the Firebase Remote Config feature (
gitlive-firebase-config,gitLiveversion bump, and Room entries if needed by the feature).
| # Firebase dependencies required by gitlive | ||
| pod 'FirebaseCore' | ||
| pod 'FirebaseRemoteConfig' | ||
| pod 'FirebaseAnalytics' | ||
| pod 'FirebaseCrashlytics' | ||
| pod 'FirebasePerformance' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Firebase iOS SDK latest version 2025 2026
💡 Result:
As of February 6, 2026, the latest Firebase iOS (Firebase Apple) SDK version is 12.9.0, released February 3, 2026. [1][2]
🌐 Web query:
gitlive firebase iOS CocoaPods dependencies requirements
💡 Result:
For GitLiveApp/firebase-kotlin-sdk on iOS, the key CocoaPods “dependency requirement” is:
- GitLive does not pull in the Firebase Apple SDK transitively on iOS — you must explicitly add the Firebase iOS pods your app uses (via CocoaPods or SPM). (github.com)
What Pods to add
Add the product-specific Firebase pods that match the GitLive modules you use, for example:
FirebaseCore(baseline)FirebaseAuth(if using auth)FirebaseFirestore(if using firestore)FirebaseFunctions, etc. (github.com)
Also note: the umbrella Firebase pod is deprecated (Firebase Apple SDK v9+); use product pods directly. (firebase.google.com)
Minimum toolchain requirements (CocoaPods install path)
Firebase’s CocoaPods distribution requires:
- Xcode 16.2+
- CocoaPods 1.12.0+
- Podfile must include
use_frameworks!(dynamic or static linkage variant) (firebase.google.com)
Kotlin Multiplatform / cocoapods-gradle specifics (if applicable)
- Set
ios.deploymentTargethigh enough; otherwise CocoaPods can fail if any pod requires a higher iOS minimum. (kotlinlang.org)
Example (Kotlin cocoapods block)
cocoapods {
pod("FirebaseCore")
pod("FirebaseFirestore") // add what you actually use
pod("FirebaseAuth")
}GitLive explicitly recommends this approach for iOS linking (and similarly for test targets). (github.com)
Citations:
- 1: https://github.com/GitLiveApp/firebase-kotlin-sdk?utm_source=openai
- 2: https://github.com/GitLiveApp/firebase-kotlin-sdk?utm_source=openai
- 3: https://firebase.google.com/docs/ios/setup?utm_source=openai
- 4: https://firebase.google.com/docs/ios/installation-methods?utm_source=openai
- 5: https://kotlinlang.org/docs/multiplatform/multiplatform-cocoapods-xcode.html?utm_source=openai
- 6: https://github.com/GitLiveApp/firebase-kotlin-sdk?utm_source=openai
🌐 Web query:
CocoaPods Firebase version pinning best practices
💡 Result:
Best practices for pinning Firebase iOS SDK versions with CocoaPods
-
Pin Firebase with a pessimistic constraint (
~>) to stay within a major (or minor) series- CocoaPods’
~>lets you accept patch updates while blocking the next breaking series (depending on how many version components you specify). (guides.cocoapods.org)
Example (stay on Firebase 10.x):
pod 'FirebaseAnalytics', '~> 10.27' pod 'FirebaseCrashlytics', '~> 10.27' pod 'FirebaseFirestore', '~> 10.27'
(Using the same
10.27family across Firebase pods reduces the chance of resolution conflicts.) - CocoaPods’
-
Avoid mixing mismatched Firebase major/minor versions across pods
- Firebase pods depend on each other with version ranges; mixing constraints can produce CocoaPods solver conflicts. (github.com)
Practical rule: choose one Firebase SDK version family (e.g.,10.27.x) and keep all Firebase pods aligned to it.
- Firebase pods depend on each other with version ranges; mixing constraints can produce CocoaPods solver conflicts. (github.com)
-
Commit
Podfile.lockand usepod installfor reproducible builds- CocoaPods explicitly recommends committing
Podfile.lock, and usingpod updateonly when you intend to change resolved versions. (guides.cocoapods.org)
Workflow: - Day-to-day / CI:
pod install - Intentional upgrades:
pod update FirebaseAnalytics(or specific pods), then test and commit updatedPodfile.lock.
- CocoaPods explicitly recommends committing
-
Only hard-pin (
= X.Y.Z) when you have a known regression and a short-term need- CocoaPods supports exact version pinning, but it increases maintenance burden (you’ll miss even patch fixes). (guides.cocoapods.org)
If you must do it:
pod 'FirebaseAnalytics', '= 10.27.0'
- CocoaPods supports exact version pinning, but it increases maintenance burden (you’ll miss even patch fixes). (guides.cocoapods.org)
-
Keep CocoaPods itself up to date enough for Firebase requirements
- Firebase noted a CocoaPods minimum (e.g., CocoaPods 1.12.0+ for privacy manifest support). (firebase.google.com)
-
Prefer the dedicated pod names (e.g.,
FirebaseAnalytics) over the older subspec style when pinning- The
Firebase/Analyticssubspec form is historically supported, but using the direct pod name is clearer for pinning and is commonly recommended. (stackoverflow.com)
- The
Citations:
- 1: https://guides.cocoapods.org/using/the-podfile.html?utm_source=openai
- 2: Cocoapods can not install between Firebase/Core and Firebase/Analytics firebase/firebase-ios-sdk#10965
- 3: https://guides.cocoapods.org/using/pod-install-vs-update.html?utm_source=openai
- 4: https://guides.cocoapods.org/using/the-podfile.html?utm_source=openai
- 5: https://firebase.google.com/support/release-notes/ios?utm_source=openai
- 6: https://stackoverflow.com/questions/59995502/how-to-specify-specific-version-of-firebase-analytics-a-pod-file-should-use?utm_source=openai
Pin Firebase pod versions and verify pod necessity.
All five Firebase pods lack version constraints, risking inconsistent builds across environments. Additionally, according to GitLive documentation, only add the product-specific Firebase pods that match the GitLive modules you use. Since this PR targets Remote Config, only FirebaseCore and FirebaseRemoteConfig are required; verify whether FirebaseAnalytics, FirebaseCrashlytics, and FirebasePerformance are dependencies of the gitlive feature or unnecessary.
Use a pessimistic constraint (~>) aligned across all Firebase pods to the current major version:
Suggested fix
- pod 'FirebaseCore'
- pod 'FirebaseRemoteConfig'
- pod 'FirebaseAnalytics'
- pod 'FirebaseCrashlytics'
- pod 'FirebasePerformance'
+ pod 'FirebaseCore', '~> 12'
+ pod 'FirebaseRemoteConfig', '~> 12'🤖 Prompt for AI Agents
In `@cmp-ios/Podfile` around lines 9 - 14, The Podfile currently declares five
Firebase pods without version constraints and likely includes unnecessary pods
for this PR; update the Podfile to (1) remove pod 'FirebaseAnalytics', pod
'FirebaseCrashlytics', and pod 'FirebasePerformance' unless
gitlive/documentation or your app features require them, keeping only pod
'FirebaseCore' and pod 'FirebaseRemoteConfig' for Remote Config support, and (2)
pin the remaining Firebase pods with a pessimistic constraint (use ~> to the
current major version used in the project) so both pod 'FirebaseCore' and pod
'FirebaseRemoteConfig' include the same ~> major.minor constraint to ensure
reproducible installs and consistent versions.
| pod 'FirebaseAnalytics' | ||
| pod 'FirebaseCrashlytics' | ||
| pod 'FirebasePerformance' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if gitlive firebase-analytics, firebase-crashlytics, or firebase-perf
# are actually declared as dependencies in any Gradle build file.
echo "=== Searching for gitlive firebase dependencies in Gradle files ==="
rg -n 'dev.gitlive' --type=gradle || rg -n 'dev.gitlive' -g '*.gradle*' -g '*.kts'
echo ""
echo "=== Searching for Firebase usage in shared/common Kotlin source ==="
rg -n 'import dev\.gitlive\.firebase' -g '*.kt' | head -40Repository: openMF/mobile-wallet
Length of output: 1844
🏁 Script executed:
#!/bin/bash
echo "=== Finding gradle build files ==="
find . -name "build.gradle*" -o -name "*.kts" | grep -E "(build|gradle)" | head -20
echo ""
echo "=== Searching for all gitlive dependencies ==="
rg -i 'gitlive' -g '*.gradle' -g '*.kts' -g '*.gradle.kts'
echo ""
echo "=== Searching for Crashlytics imports ==="
rg -n 'firebase.*crashlytics|crashlytics.*firebase' -g '*.kt' -i
echo ""
echo "=== Searching for Performance imports ==="
rg -n 'firebase.*performance|performance.*firebase' -g '*.kt' -i
echo ""
echo "=== Examining cmp-ios/Podfile ==="
cat cmp-ios/Podfile | head -40Repository: openMF/mobile-wallet
Length of output: 2755
Remove FirebasePerformance pod — it's not declared as a dependency.
The dev.gitlive:firebase-analytics and dev.gitlive:firebase-crashlytics dependencies are legitimately used in the core-base/analytics module, so keeping FirebaseAnalytics and FirebaseCrashlytics is justified.
However, FirebasePerformance is not declared anywhere in the Gradle build files (core-base/analytics/build.gradle.kts does not declare libs.gitlive.firebase.performance), and no imports of it appear in the Kotlin codebase. Remove it to avoid unnecessary binary size and attack surface.
🤖 Prompt for AI Agents
In `@cmp-ios/Podfile` around lines 12 - 14, Remove the unused FirebasePerformance
pod entry from the Podfile: delete the line "pod 'FirebasePerformance'" and keep
"pod 'FirebaseAnalytics'" and "pod 'FirebaseCrashlytics'"; after removing, run
pod install (or bundle exec pod install) to update the lockfile and ensure there
are no residual references to FirebasePerformance in the iOS build artifacts.
| androidGradlePlugin = "8.12.3" | ||
| androidTools = "31.12.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify AGP 8.12.3 exists on Maven Central
echo "=== Checking AGP 8.12.3 ==="
curl -s -o /dev/null -w "Status: %{http_code}\n" "https://repo1.maven.org/maven2/com/android/tools/build/gradle/8.12.3/gradle-8.12.3.pom"
# Also check android tools 31.12.3
echo "=== Checking Android Tools 31.12.3 ==="
curl -s -o /dev/null -w "Status: %{http_code}\n" "https://repo1.maven.org/maven2/com/android/tools/common/31.12.3/common-31.12.3.pom"
# Check what 8.x versions exist (list the most recent)
echo "=== Available 8.x AGP versions ==="
curl -s "https://repo1.maven.org/maven2/com/android/tools/build/gradle/maven-metadata.xml" 2>/dev/null | grep -oP '<version>8\.\d+\.\d+</version>' | tail -30
# Also check 9.x for reference
echo "=== Available 9.x AGP versions ==="
curl -s "https://repo1.maven.org/maven2/com/android/tools/build/gradle/maven-metadata.xml" 2>/dev/null | grep -oP '<version>9\.\d+\.\d+</version>' | tail -10Repository: openMF/mobile-wallet
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Try fetching the raw metadata directly without parsing
echo "=== AGP Metadata (raw) ==="
curl -s "https://repo1.maven.org/maven2/com/android/tools/build/gradle/maven-metadata.xml" 2>/dev/null | head -100
echo ""
echo "=== Available AGP versions via alternative check ==="
curl -s "https://repo1.maven.org/maven2/com/android/tools/build/gradle/" 2>/dev/null | grep -oP 'href="[0-9]+\.[0-9]+\.[0-9]+' | grep -oP '[0-9]+\.[0-9]+\.[0-9]+' | tail -30Repository: openMF/mobile-wallet
Length of output: 3036
Fix non-existent Gradle plugin versions: androidGradlePlugin = "8.12.3" and androidTools = "31.12.3" do not exist on Maven Central.
Both versions return HTTP 404 errors. Direct Maven Central repository checks confirm neither com.android.tools.build:gradle:8.12.3 nor com.android.tools.common:31.12.3 exist as published releases. Gradle sync will fail with these versions. Use actual published AGP versions (e.g., 8.1.x, 8.2.x, or 9.0.x based on your target API level and compatibility requirements).
🤖 Prompt for AI Agents
In `@gradle/libs.versions.toml` around lines 6 - 7, The listed versions in
libs.versions.toml are invalid and will cause Gradle sync failures; update
androidGradlePlugin and androidTools to actual published AGP/tool versions
(replace androidGradlePlugin = "8.12.3" and androidTools = "31.12.3" with a real
release such as a published 8.1.x / 8.2.x / 9.0.x for androidGradlePlugin and
the matching published androidTools coordinate), verify the chosen versions
exist on Maven Central, and run a Gradle sync to confirm compatibility (look for
androidGradlePlugin and androidTools in the file to make the changes).
| filekit-core = { group = "io.github.vinceglb", name = "filekit-core", version.ref = "fileKit" } | ||
| filekit-compose = { module = "io.github.vinceglb:filekit-dialogs-compose", version.ref = "fileKit" } | ||
| filekit-dialogs = { module = "io.github.vinceglb:filekit-dialogs", version.ref = "fileKit" } | ||
| filekit-dialogs-compose = { module = "io.github.vinceglb:filekit-dialogs-compose", version.ref = "fileKit" } | ||
| filekit-coil = { module = "io.github.vinceglb:filekit-coil", version.ref = "fileKit" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. First, locate and examine gradle/libs.versions.toml
find . -name "libs.versions.toml" -type fRepository: openMF/mobile-wallet
Length of output: 91
🏁 Script executed:
# 2. Once we find the file, let's examine the specific lines mentioned (349-353)
# and surrounding context to understand the state
head -360 gradle/libs.versions.toml | tail -20Repository: openMF/mobile-wallet
Length of output: 1986
🏁 Script executed:
# 3. Search for all usages of filekit-compose and filekit-dialogs-compose in build files
rg -n 'filekit-compose|filekit-dialogs-compose' --glob '*.kts' --glob '*.gradle'Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
# 4. Also check if there are any version references or other filekit entries
rg -n 'fileKit' gradle/libs.versions.toml -A 2 -B 2Repository: openMF/mobile-wallet
Length of output: 1144
🏁 Script executed:
# Search for any usage of filekit-compose or filekit-dialogs-compose in all files
rg -n 'filekit-compose|filekit-dialogs-compose' --type-add 'gradle:*.{gradle,kts,toml}'Repository: openMF/mobile-wallet
Length of output: 1154
🏁 Script executed:
# Also search for any gradle dependency declarations that might use these
rg -n 'libs\.filekit'Repository: openMF/mobile-wallet
Length of output: 1365
🏁 Script executed:
# Check if these aliases appear in any build configuration or documentation
rg -i 'filekit-compose|filekit-dialogs-compose' --glob '*.md' --glob '*.txt' --glob '*.properties'Repository: openMF/mobile-wallet
Length of output: 856
Consolidate duplicate filekit library aliases: filekit-compose and filekit-dialogs-compose resolve to the same Maven module.
Both aliases (lines 350 and 352) point to io.github.vinceglb:filekit-dialogs-compose. The filekit-compose alias is actively used in the codebase (core-base/ui/build.gradle.kts lines 58, 72), while filekit-dialogs-compose is used elsewhere. This duplication creates unnecessary ambiguity.
Migrate the 2 usages of libs.filekit.compose to libs.filekit.dialogs.compose in core-base/ui/build.gradle.kts, then remove the duplicate filekit-compose entry from gradle/libs.versions.toml.
🤖 Prompt for AI Agents
In `@gradle/libs.versions.toml` around lines 349 - 353, Two aliases in
gradle/libs.versions.toml point to the same module: filekit-compose and
filekit-dialogs-compose both map to io.github.vinceglb:filekit-dialogs-compose;
update code to use the canonical alias and remove the duplicate. Replace all
usages of libs.filekit.compose with libs.filekit.dialogs.compose in the
build.gradle.kts that currently imports filekit (where libs.filekit.compose
appears), then delete the redundant filekit-compose entry from
gradle/libs.versions.toml so only filekit-dialogs-compose remains.
- Update compileSdk from 35 to 36 (required by newer AndroidX libraries) - Replace deprecated Firebase KTX imports with new Firebase API - Update Firebase library names from -ktx to non-ktx variants - Regenerate dependency guard baseline Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmp-shared/build.gradle.kts`:
- Line 84: Remove the redundant local compileSdk declaration: delete the line
setting compileSdk = 36 from this module so the project uses the centralized
value defined by the convention plugin (KotlinAndroid). Ensure no other local
compileSdk assignments remain in the file so the convention (KotlinAndroid)
controls the SDK version.
| android { | ||
| namespace = "org.mifospay.shared" | ||
| compileSdk = 35 | ||
| compileSdk = 36 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check what compileSdk the convention plugin sets
echo "=== Convention plugin compileSdk ==="
fd -e kt -e kts --full-path 'build-logic|convention' | xargs rg -n 'compileSdk' 2>/dev/null || echo "No convention plugin compileSdk found"
echo ""
echo "=== All compileSdk declarations across modules ==="
fd -e kts --glob 'build.gradle*' | xargs rg -n 'compileSdk' 2>/dev/null || echo "No compileSdk declarations found"
echo ""
echo "=== libs.versions.toml SDK version catalog entries ==="
fd 'libs.versions.toml' | xargs rg -n -i 'compileSdk\|android.*sdk\|androidCompileSdk' 2>/dev/null || echo "No version catalog entries found"
echo ""
echo "=== Check cmp-shared/build.gradle.kts context around line 84 ==="
cat -n cmp-shared/build.gradle.kts | head -90 | tail -20Repository: openMF/mobile-wallet
Length of output: 945
Remove the redundant compileSdk = 36 declaration; it's already configured by the convention plugin.
The convention plugin (build-logic/convention/src/main/kotlin/org/mifospay/KotlinAndroid.kt:22) already sets compileSdk = 36 for all modules. The hardcoded declaration here is redundant and should be removed to keep configuration centralized.
🤖 Prompt for AI Agents
In `@cmp-shared/build.gradle.kts` at line 84, Remove the redundant local
compileSdk declaration: delete the line setting compileSdk = 36 from this module
so the project uses the centralized value defined by the convention plugin
(KotlinAndroid). Ensure no other local compileSdk assignments remain in the file
so the convention (KotlinAndroid) controls the SDK version.
3 participants
Issue Fix
Fixes #{Issue Number}
Jira Task: Task_Number
Screenshots
Screen.Recording.2025-11-26.at.9.49.40.AM.mov
Description
Apply the
AndroidStyle.xmlstyle template to your code in Android Studio.Run the unit tests with
./gradlew checkto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit