Supabase remote config instance loader #1945
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
}
]
}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a multi-platform server instance selector: gesture trigger, bottom-sheet UI, ViewModel with remote-config and prefs integration, persistence of main/interbank selections, network DI wiring to use chosen instances, models/resources, and dependency 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: read selectedInstance / selectedInterbankInstance
Preferences-->>VM: StateFlow values
VM-->>Screen: render state (instances, selections)
User->>Screen: select instance + tap Update
Screen->>VM: handleAction(UpdateInstances)
VM->>Preferences: updateSelectedInstance/updateSelectedInterbankInstance
Preferences-->>VM: DataState.Success
VM-->>Screen: emit DismissSheet
Screen->>App: onDismiss()
App->>Network: InstanceConfigManager -> new selected instance used 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
Outdated
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
Outdated
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
Outdated
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).
- 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.
- Create InterbankServer model with endpoint, protocol, path, label, isDefault - Update ServerInstance to include interbankServers list and remove type field - Simplify InstancesConfig by removing separate main/interbank methods - Update UserPreferencesDataSource/Repository to use InterbankServer type - Redesign InstanceSelectorScreen with nested UI showing interbank servers below selected main instance - Add beautified InterbankServerItem with animations and improved styling - Update InstanceConfigManager to work with new InterbankServer type - Update fallback JSON config with nested structure - Update Supabase app_config table with new nested data 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/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt`:
- Around line 86-92: InstanceSelectorScreen currently applies the incoming
modifier twice: once to MifosBottomSheet and again to the inner MifosScaffold;
remove the duplicate application by keeping the incoming modifier only on
MifosBottomSheet and replace the MifosScaffold modifier with Modifier (or a new
local innerModifier) so external sizing/padding is not applied twice; update
InstanceSelectorScreen to pass modifier to MifosBottomSheet and use Modifier (or
innerModifier) for the MifosScaffold to avoid double application.
🧹 Nitpick comments (5)
core/network/src/commonMain/kotlin/org/mifospay/core/network/config/InstanceConfigManager.kt (2)
74-77:getSelfServiceUrl()duplicatesfullUrllogic — use computed property instead.
fullUrlalready computes"$protocol$endpoint$path". This method manually re-concatenates the same pieces.♻️ Suggested simplification
fun getSelfServiceUrl(): String { val instance = getCurrentInstance() - return "${instance.protocol}${instance.endpoint}${instance.path}self/" + return "${instance.fullUrl}self/" }
20-48:DEFAULT_INTERBANK_INSTANCEduplicates the interbank entry insideDEFAULT_MAIN_INSTANCE.The same interbank server definition exists in both
DEFAULT_MAIN_INSTANCE.interbankServers[0]andDEFAULT_INTERBANK_INSTANCE. Consider defining it once and referencing it in both places to avoid drift.♻️ Suggested refactor
companion object { + private val DEFAULT_INTERBANK_INSTANCE = InterbankServer( + endpoint = "apis.flexcore.mx", + protocol = "https://", + path = "/v1.0/vnext2/", + label = "Default Interbank", + isDefault = true, + ) + private val DEFAULT_MAIN_INSTANCE = ServerInstance( endpoint = "mifos-bank-2.mifos.community", protocol = "https://", path = "/fineract-provider/api/v1/", platformTenantId = "mifos-bank-2", label = "Default Instance", isDefault = true, - interbankServers = listOf( - InterbankServer( - endpoint = "apis.flexcore.mx", - protocol = "https://", - path = "/v1.0/vnext2/", - label = "Default Interbank", - isDefault = true, - ), - ), + interbankServers = listOf(DEFAULT_INTERBANK_INSTANCE), ) - - private val DEFAULT_INTERBANK_INSTANCE = InterbankServer( - endpoint = "apis.flexcore.mx", - protocol = "https://", - path = "/v1.0/vnext2/", - label = "Default Interbank", - isDefault = true, - ) }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt (2)
214-321: Inconsistent theming:MainInstanceItemmixesMaterialThemeandKptTheme.
MainInstanceItemusesMaterialTheme.typographyandMaterialTheme.colorScheme(e.g., Lines 256, 272, 276, 290, 293, 301, 304), whileInterbankServerItemconsistently usesKptTheme.typographyandKptTheme.colorScheme. This inconsistency may cause visual discrepancies ifMaterialThemeandKptThemediverge. Consider aligning on one theme system.
513-683: Preview composables use inconsistent theme wrappers.Some previews wrap in
KptTheme { }(Lines 516, 574) while others useMifosTheme { }(Lines 552, 608). SinceMifosThemesets upKptThemeinternally (viaKptMaterialTheme), usingKptThemedirectly in previews may skip color scheme and gradient setup fromMifosTheme. Standardize onMifosThemefor all previews to ensure they reflect production theming.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorViewModel.kt (1)
114-116:ConfigLoadedaction is dead code — defined, accepted, but never dispatched or meaningfully handled.
InstanceSelectorAction.ConfigLoadedis declared in the sealed interface and matched inhandleAction, but the handler is a no-op comment (// Handled by init block), and config loading is indeed driven entirely by thecombineininit. This dead action adds unnecessary surface area to the public API and may confuse consumers.Remove it unless there's a planned use case.
Suggested fix
// In handleAction: - is InstanceSelectorAction.ConfigLoaded -> { - // Handled by init block - } // In InstanceSelectorAction: - data class ConfigLoaded(val config: DataState<InstancesConfig>) : InstanceSelectorActionAlso applies to: 172-172
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/instance/InstanceSelectorScreen.kt
Show resolved
Hide resolved
- Add CocoaPods source - Add Firebase pods (Core, Analytics, Crashlytics) - Add post_install hook to disable code signing for Pod targets - Update dependency guard baseline Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add SupabaseInstanceConfigLoader for fetching instance config from Supabase - Add SupabaseConfigConventionPlugin for build configuration - Update InstanceConfigLoader to use Supabase as primary source with fallback - Update NetworkModule with Supabase dependencies - Update analytics module to use gitlive firebase-analytics - Remove deprecated FirebaseAnalyticsHelper - Update iOS project configuration and Podfile.lock - Update google-services.json configuration - Update library versions in libs.versions.toml Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ilds - Added Firebase client entry for org.mifospay.debug package - Updated prodRelease-badging.txt with compileSdk 36 and version updates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Added SUPABASECREDS secret documentation to workflow files - Pass supabase_creds to reusable workflows for Supabase integration - Secret should be Base64 encoded JSON with url and anonKey fields Note: mifos-x-actionhub reusable workflows need to be updated to decode and write the credentials file before Gradle builds. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Updated all workflow references to use v1.0.9 which includes Supabase credentials support for KMP projects. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reverted the following files to development branch state: - cmp-ios/iosApp/iOSApp.swift - cmp-ios/iosApp.xcodeproj/xcshareddata/xcschemes/cmp-ios.xcscheme - cmp-ios/Podfile - core-base/analytics AnalyticsModule.kt files - Restored FirebaseAnalyticsHelper.kt Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Gradle's inputs.file().optional() doesn't work as expected - it still requires the file to exist. Fix by only declaring the file as input when it actually exists. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Regenerated yarn.lock to fix CI build failure. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
therajanmaurya
changed the title
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