chore: add missing modules from kmp template#3109
chore: add missing modules from kmp template#3109amanna13 wants to merge 3 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughAdds a new multiplatform HTTP networking layer and analytics module, multiple platform-specific client implementations, extensive CI/CD workflow updates, iOS deployment and setup scripts, image/Share utilities refactor, various build-logic and dependency adjustments across Android, iOS, web, desktop, and Gradle plugin catalog. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application/Composable
participant HttpFactory as setupDefaultHttpClient
participant HttpClient as HttpClient(platform)
participant Backend as Remote API
participant Converter as ResultSuspendConverterFactory
App->>HttpFactory: request configured client (baseUrl, auth, logging)
HttpFactory->>HttpClient: instantiate platform HttpClient with config
App->>HttpClient: perform request
HttpClient->>Backend: send HTTP request
Backend-->>HttpClient: HTTP response/status
HttpClient->>Converter: provide response
Converter-->>App: NetworkResult<Success|Error>
sequenceDiagram
participant UI as Composable UI
participant Tracker as MifosAnalyticsTracker
participant Helper as AnalyticsHelper
participant Backend as Analytics Backend
UI->>Tracker: trackDomainEvent(...)
Tracker->>Helper: logEvent(name, params)
Helper->>Backend: send event(payload)
Backend-->>Helper: ack
Helper-->>Tracker: success
Tracker-->>UI: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
🚥 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)
No actionable comments were generated in the recent review. 🎉 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: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
core-base/ui/README.md (1)
389-420:⚠️ Potential issue | 🟡 MinorFix missing
contextin updated image-loading snippets.After switching to
rememberImageLoader(), both snippets still callrememberImageRequest(context, url)but nocontextis defined, so the example no longer compiles. Update the docs to either show howcontextis obtained (e.g.,val context = LocalPlatformContext.current) or use a context-freerememberImageRequestoverload if that’s the new API.✏️ Suggested doc tweak
`@Composable` fun CircularProfileImage(url: String, size: Dp = 48.dp) { + val context = LocalPlatformContext.current val imageLoader = rememberImageLoader() AsyncImage( model = rememberImageRequest(context, url), contentDescription = "Profile picture", @@ `@Composable` fun BackgroundImage(url: String, overlay: Color = Color.Black.copy(alpha = 0.3f)) { + val context = LocalPlatformContext.current val imageLoader = rememberImageLoader() Box { AsyncImage( model = rememberImageRequest(context, url),build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt (1)
46-53:⚠️ Potential issue | 🟡 MinorDuplicate dependency:
koin.androidx.composeis added twice.Line 48 and line 52 both add
koin.androidx.composetoandroidMainImplementation. Additionally, lines 46–48 and 50–53 appear to be two overlapping blocks of Koin dependencies that should be consolidated.Proposed fix — deduplicate the Koin blocks
add("androidMainImplementation", platform(libs.findLibrary("koin-bom").get())) add("androidMainImplementation", libs.findLibrary("koin-android").get()) add("androidMainImplementation", libs.findLibrary("koin.androidx.compose").get()) add("androidMainImplementation", libs.findLibrary("koin.android").get()) add("androidMainImplementation", libs.findLibrary("koin.androidx.navigation").get()) - add("androidMainImplementation", libs.findLibrary("koin.androidx.compose").get()) add("androidMainImplementation", libs.findLibrary("koin.core.viewmodel").get())core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/PerformanceTracker.kt (2)
265-265:⚠️ Potential issue | 🔴 CriticalCritical bug:
currentTimeis a top-levelvalevaluated only once at initialization.This property is computed a single time when the file is loaded and never re-evaluated. Every reference to
currentTime(lines 29, 30, 49, 190, 200, 217, 232) returns the same fixed timestamp, meaning all timer durations will always be0.It should be either a function or a
valwith a custom getter:🐛 Proposed fix
-private val currentTime = Clock.System.now().toEpochMilliseconds() +private val currentTime get() = Clock.System.now().toEpochMilliseconds()
28-40:⚠️ Potential issue | 🟠 MajorTimer IDs are not unique since
currentTimeis static (same root cause as above).Because
currentTimeis always the same value, the timer ID"${operationName}_$currentTime"will collide ifstartTimeris called more than once for the same operation. This meansactiveTimers[timerId]will overwrite previous entries, silently losing timers. Once thecurrentTimefix is applied, there's still a small chance of collisions at millisecond granularity under concurrent use—consider using an atomic counter or UUID instead.core-base/analytics/build.gradle.kts (1)
29-30:⚠️ Potential issue | 🟠 MajorRemove stale
kotlinx-datetimedependency and update comment.
PerformanceTracker.ktuseskotlin.time.Clockfor timing, notkotlinx.datetime. Sincekotlinx-datetimeis not used anywhere in this module, the dependency (lines 29-30) and its comment are unnecessary and should be removed.build-logic/convention/build.gradle.kts (2)
9-9:⚠️ Potential issue | 🟡 MinorStale comment references JDK 17, but the actual target is JDK 21.
The comment says "target JDK 17" while
JavaVersion.VERSION_21andJvmTarget.JVM_21are used on lines 12–18.Proposed fix
-// Configure the build-logic plugins to target JDK 17 +// Configure the build-logic plugins to target JDK 21
28-33:⚠️ Potential issue | 🟡 MinorDuplicate
compileOnly(libs.room.gradlePlugin)dependency.
libs.room.gradlePluginis declared on both line 28 and line 33. Remove one to avoid confusion.Proposed fix
compileOnly(libs.room.gradlePlugin) compileOnly(libs.detekt.gradlePlugin) compileOnly(libs.ktlint.gradlePlugin) compileOnly(libs.spotless.gradlePlugin) implementation(libs.truth) - compileOnly(libs.room.gradlePlugin) compileOnly(libs.firebase.crashlytics.gradlePlugin)core-base/database/build.gradle.kts (1)
1-20:⚠️ Potential issue | 🟡 MinorDuplicate copyright headers and a stale import.
There are two copyright blocks (2026 on lines 1–9 and 2025 on lines 12–20) — keep only one. Also, the
import org.jetbrains.compose.composeon line 10 appears unused in a database module and should be removed.Proposed fix
/* * Copyright 2025 Mifos Initiative * * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. * - * See https://github.com/openMF/mobile-mobile/blob/master/LICENSE.md + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSE */ -import org.jetbrains.compose.compose - -/* - * Copyright 2025 Mifos Initiative - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. - * - * See https://github.com/openMF/kmp-project-template/blob/main/LICENSE - */ plugins {core-base/ui/build.gradle.kts (1)
1-20:⚠️ Potential issue | 🟡 MinorDuplicate copyright headers — same issue as
core-base/database/build.gradle.kts.Two copyright blocks are present (2026 and 2025). Keep only one. The
import org.jetbrains.kotlin.gradle.ExperimentalKotlinGradlePluginApion line 10 also appears unused in this file.core-base/platform/build.gradle.kts (2)
1-20:⚠️ Potential issue | 🟡 MinorDuplicate copyright headers and misplaced import.
There are two copyright blocks (lines 1–9 and lines 12–20), with an
importstatement sandwiched between them. This looks like a merge artifact. Remove the duplicate header.Proposed fix
/* * Copyright 2026 Mifos Initiative * * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. * * See https://github.com/openMF/mobile-mobile/blob/master/LICENSE.md */ import org.gradle.kotlin.dsl.implementation -/* - * Copyright 2025 Mifos Initiative - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. - * - * See https://github.com/openMF/kmp-project-template/blob/main/LICENSE - */ plugins {
27-28:⚠️ Potential issue | 🟠 MajorNamespace uses
templateprefix — appears to be an unreplaced placeholder across multiple core-base modules.
"template.core.base.platform"appears to be carried over from the KMP template project. This is not unique to platform; seven of eight core-base modules use thetemplate.core.base.*pattern (ui, designsystem, datastore, database, common, analytics, and platform), while onlycore-base/networkuses"org.mifos.corebase.network". Decide whether thetemplateprefix should be replaced project-wide with the org.mifos namespace, and apply the correction consistently across all affected modules.core-base/datastore/build.gradle.kts (1)
15-17:⚠️ Potential issue | 🟠 MajorAll
core-base/modules use incorrecttemplate.core.base.*namespace prefix.The namespace
"template.core.base.datastore"should be updated to"org.mifos.mobile.core.base.datastore"to match the project's actual namespace convention used across allcore/*modules. This issue affects all 8 core-base modules (datastore, ui, platform, database, designsystem, analytics, common, network) which all incorrectly use thetemplateprefix.cmp-ios/iosApp.xcodeproj/project.pbxproj (3)
377-377:⚠️ Potential issue | 🟠 MajorHardcoded
DEVELOPMENT_TEAMleaks a personal Apple Developer Team ID.
L432S2FZP5appears to be an individual's Apple Developer Team ID. This should remain parameterized viaConfig.xcconfig(as it was before with${TEAM_ID}), so other contributors can build with their own team and the value isn't committed to source control.Also applies to: 410-410
379-382:⚠️ Potential issue | 🔴 Critical
FRAMEWORK_SEARCH_PATHSis inconsistent between Debug and Release.In Debug (line 381), the two framework search paths are concatenated on a single entry separated by a literal
\n:"$(SRCROOT)/../shared/build/xcode-frameworks/$(CONFIGURATION)/$(SDK_NAME)\n$(SRCROOT)/../composeApp/build/xcode-frameworks/$(CONFIGURATION)/$(SDK_NAME)",In Release (lines 414–415), they are correctly listed as two separate array entries. The Debug entry will likely produce an invalid search path at build time.
Proposed fix for Debug FRAMEWORK_SEARCH_PATHS
FRAMEWORK_SEARCH_PATHS = ( "$(inherited)", - "$(SRCROOT)/../shared/build/xcode-frameworks/$(CONFIGURATION)/$(SDK_NAME)\n$(SRCROOT)/../composeApp/build/xcode-frameworks/$(CONFIGURATION)/$(SDK_NAME)", + "$(SRCROOT)/../shared/build/xcode-frameworks/$(CONFIGURATION)/$(SDK_NAME)", + "$(SRCROOT)/../composeApp/build/xcode-frameworks/$(CONFIGURATION)/$(SDK_NAME)", );Also applies to: 412-416
247-249:⚠️ Potential issue | 🔴 CriticalTarget-level configs reference non-existent Pods xcconfigs without
#includechaining — builds will lose Config.xcconfig settings whenpod installis run.The project-level Debug/Release configurations (lines 247-249, 311-313) use
Config.xcconfigas their base, which definesTEAM_ID,BUNDLE_ID, andAPP_NAME. However, the target-level configurations (lines 369-371, 402-404) referencePods-iosApp.debug.xcconfigandPods-iosApp.release.xcconfig— files that don't yet exist and will be auto-generated by CocoaPods onpod install.The
Podfilecontains no configuration to make the generated Pods xcconfigs#includeor reference yourConfig.xcconfig. When CocoaPods generates these files, they will not chain back to your user settings, causing the target build to loseTEAM_ID,BUNDLE_ID, andAPP_NAMEentirely.Solution: Either (1) revert target-level configs to reference
Config.xcconfigdirectly, or (2) create per-configuration user xcconfigs that#includeboth the generated Pods xcconfig andConfig.xcconfig, and set those as the target-level base configurations.
🤖 Fix all issues with AI agents
In
`@build-logic/convention/src/main/kotlin/KMPCoreBaseLibraryConventionPlugin.kt`:
- Around line 29-31: The PR sets defaultConfig.targetSdk = 36 while
configureKotlinAndroid(...) configures compileSdk = 35, which violates Android's
requirement targetSdk ≤ compileSdk; either set defaultConfig.targetSdk to 35
here or update the compileSdk in KotlinAndroid.kt to 36. Locate the call site
configureKotlinAndroid in KMPCoreBaseLibraryConventionPlugin.kt and change
defaultConfig.targetSdk to 35 (or alternatively bump the compileSdk
constant/value in KotlinAndroid.kt to 36) so targetSdk and compileSdk are
consistent.
In `@cmp-web/src/jsMain/kotlin/Application.kt`:
- Around line 36-62: The localeVersion state (var localeVersion by remember {
mutableStateOf(0) }) used as the key for the key(localeVersion) block is never
incremented, so changing locale in handleAppLocale doesn't trigger
recomposition; update the handleAppLocale lambda in Application.kt (the one
passed to SharedApp) to increment localeVersion (localeVersion++) after you
modify localStorage and document.documentElement attributes (do this in both the
non-null languageTag branch and the else branch) so key(localeVersion) forces a
full recomposition when the locale changes.
In `@cmp-web/src/wasmJsMain/kotlin/Main.kt`:
- Around line 57-87: The issue is that localeVersion (used with
key(localeVersion) to force recomposition) is never incremented so locale
changes don't trigger recomposition; inside the handleAppLocale lambda passed to
SharedApp, after updating localStorage and document.documentElement (and after
the else branch actions), increment localeVersion (i.e., localeVersion++ or
localeVersion = localeVersion + 1) so key(localeVersion) will change and force a
full recomposition; locate the localeVersion variable declaration and the
SharedApp(handleAppLocale = { ... }) callback to make this change.
In
`@core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/AnalyticsExtensions.kt`:
- Line 12: Replace the usage of kotlin.time.Clock and
kotlin.time.Instant.toEpochMilliseconds (which are not available in Kotlin
2.1.20) with the stable kotlinx.datetime equivalents: use kotlinx.datetime.Clock
and kotlinx.datetime.Instant.toEpochMilliseconds(), or alternatively upgrade the
project Kotlin version to 2.3+ so kotlin.time APIs are available; if you choose
to keep kotlin.time on older compiler, add `@OptIn`(ExperimentalTime::class) to
the relevant functions/classes (e.g., in AnalyticsExtensions.kt) to suppress
experimental API errors—pick one approach and apply it consistently to all sites
that call toEpochMilliseconds (lines referencing Instant.toEpochMilliseconds and
imports of kotlin.time.Clock).
In
`@core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt`:
- Around line 88-98: In ResultSuspendConverterFactory update the success-range
check from 200..209 to 200..299 and handle 204 No Content before calling
result.response.body(successType): if status is 204, return
NetworkResult.Success with an appropriate empty/null value for successType
instead of invoking body() (which triggers
NoTransformationFoundException/SerializationException and yields
NetworkError.SERIALIZATION); otherwise call result.response.body(successType)
and wrap in NetworkResult.Success, preserving the existing catches for
NoTransformationFoundException and SerializationException that return
NetworkResult.Error(NetworkError.SERIALIZATION).
In
`@core/analytics/src/commonMain/kotlin/org/mifos/core/analytics/MifosComposeAnalytics.kt`:
- Around line 94-102: trackMifosAction currently replaces any existing click
handler with an empty clickable, dropping events and breaking UI; change
Modifier.trackMifosAction to NOT add a clickable (simply return this) and
instead implement safe wrapper overloads for the public extensions
(trackClientAction, trackLoanAction, trackSavingsAction) that accept the
original onClick lambda and return Modifier.clickable { recordAnalytics(...) ;
originalOnClick() } (or a variant that wraps the provided onClick), or
alternatively call the analytics helper from inside the component's existing
click handlers — ensure the symbols Modifier.trackMifosAction,
trackClientAction, trackLoanAction, and trackSavingsAction are updated so
analytics is recorded but existing click behavior is preserved and not shadowed.
In `@scripts/deploy_appstore.sh`:
- Around line 266-310: The script uses set -e which causes eval $FASTLANE_CMD to
exit the script on failure, making the subsequent if [ $? -eq 0 ] branch
unreachable; change the flow so you capture the fastlane exit code instead of
allowing set -e to terminate execution: either temporarily disable set -e before
eval $FASTLANE_CMD and re-enable it after, or run eval $FASTLANE_CMD in a way
that prevents immediate exit (e.g., append a no-fail guard) and store its exit
status into a variable (e.g., FASTLANE_EXIT_CODE) then replace references to $?
in the success/failure checks with that variable; target the eval $FASTLANE_CMD
invocation and the if [ $? -eq 0 ] conditional in deploy_appstore.sh (and apply
the same change to the analogous locations in deploy_firebase.sh and
deploy_testflight.sh).
In `@scripts/deploy_firebase.sh`:
- Around line 150-165: Because the script uses set -e, the if/else that checks
"$?" after running "bundle exec fastlane ios deploy_on_firebase" is dead code;
change to capture the fastlane exit code immediately (e.g., run the command,
assign its exit status to a variable like deploy_exit=$?), then use that
variable in the conditional instead of relying on "$?" later. Update the
conditional that references "$?" to check the saved variable (e.g., deploy_exit)
and keep the same success and failure branches
(print_section/print_success/print_error/print_info and exit 1) so the failure
branch is reachable despite set -e.
In `@scripts/deploy_testflight.sh`:
- Around line 182-206: The failure branch is unreachable due to set -e causing
the script to exit on a failing command; change the invocation so the script can
test its result instead of relying on $? after a failing command. Replace the
plain "bundle exec fastlane ios beta" + subsequent "$? -eq 0" check with a
conditional run like "if bundle exec fastlane ios beta; then" ... "else" ...
"fi" (so the else block with print_section/print_error/print_info runs on
failure) or alternatively run the command and capture its exit code explicitly
(e.g., "bundle exec fastlane ios beta || exit_code=$?") before checking
exit_code; update uses of print_section, print_success, print_error accordingly.
In `@scripts/verify_apn_setup.sh`:
- Line 9: The script uses post-increment arithmetic like ((ERRORS++)) and
((WARNINGS++)), which returns exit code 1 when the old value is 0 and causes set
-e to abort; replace those occurrences (the ((ERRORS++)) and ((WARNINGS++))
expressions) with a non-zero-evaluating increment such as ((++ERRORS)) and
((++WARNINGS)) (or alternately ERRORS=$((ERRORS+1)) / WARNINGS=$((WARNINGS+1))
or ((ERRORS+=1)) || true) so increments never evaluate to 0 and set -e will not
prematurely exit.
In `@scripts/verify_ios_deployment.sh`:
- Line 9: The script currently uses set -e which causes the first failing
check() to abort execution and prevents the consolidated report using
PASSED_CHECKS/FAILED_CHECKS/WARNING_CHECKS from being printed; either remove the
set -e line, or modify calls to check (the check function) to be invoked as
"check ... || true" so failures don't trigger exit, or change check() to always
return 0 while still updating FAILED_CHECKS; update invocations that rely on set
-e accordingly so the summary printing block still runs.
🟠 Major comments (28)
scripts/pre-commit.sh-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorBranch protection no longer guards
development.Replacing
"development"with"dev"means contributors can now commit directly to thedevelopmentbranch, which is the main integration branch per the project's contribution guidelines. If the intent is to protectdevas an additional branch, it should be added alongsidedevelopment, not replace it.Proposed fix
- if [ "$CURRENT_BRANCH" = "master" ] || [ "$CURRENT_BRANCH" = "dev" ]; then + if [ "$CURRENT_BRANCH" = "master" ] || [ "$CURRENT_BRANCH" = "development" ] || [ "$CURRENT_BRANCH" = "dev" ]; thenBased on learnings: "Always use feature branches; NEVER push directly to
developmentbranch."cmp-shared/src/nativeMain/kotlin/org/mifos/shared/ViewController.kt-10-10 (1)
10-10: 🛠️ Refactor suggestion | 🟠 MajorRemove dead
applyIosLocalefunction instead of suppressing the warning.The
@file:Suppress("UnusedPrivateMember")annotation was added to silence the warning forapplyIosLocale, which is no longer called — thehandleAppLocalelambda (lines 36–43) inlines the same logic. Suppressing the warning masks genuinely unused code and could hide future issues. Delete the function and the annotation.♻️ Proposed fix
-@file:Suppress("UnusedPrivateMember") - package org.mifos.shared} -private fun applyIosLocale( - languageTag: String?, -) { - if (!languageTag.isNullOrBlank()) { - NSUserDefaults.standardUserDefaults.setObject( - listOf(languageTag), - forKey = "AppleLanguages", - ) - } else { - NSUserDefaults.standardUserDefaults.removeObjectForKey("AppleLanguages") - } -}Also applies to: 48-59
cmp-shared/src/nativeMain/kotlin/org/mifos/shared/ViewController.kt-27-34 (1)
27-34:⚠️ Potential issue | 🟠 MajorReplace deprecated
keyWindowAPI with scene-aware window access.
UIApplication.sharedApplication.keyWindowis deprecated since iOS 13. With multi-scene support, this can returnnilunexpectedly or target the wrong scene. UseconnectedSceneswith the foreground-activeUIWindowSceneinstead:UIApplication.sharedApplication.connectedScenes .compactMap { $0 as? UIWindowScene } .first { $0.activationState == UISceneActivationStateForegroundActive } ?.keyWindow?.overrideUserInterfaceStyle = styleThis pattern is forward-compatible with iOS 13+ (Compose Multiplatform's minimum) and handles multi-window/iPad scenarios correctly.
cmp-shared/src/commonMain/kotlin/cmp/shared/SharedApp.kt-28-37 (1)
28-37:⚠️ Potential issue | 🟠 Major
getDefaultImageLoaderis called on every recomposition — should be remembered.
getDefaultImageLoader(...)builds a newImageLoader(with memory cache, network cache, etc.) each timeSharedApprecomposes. This is wasteful and can cause cache thrashing. Wrap it inrememberkeyed on the platform context.🔧 Proposed fix
+import androidx.compose.runtime.remember + `@Composable` fun SharedApp( handleThemeMode: (osValue: Int) -> Unit, handleAppLocale: (locale: String?) -> Unit, modifier: Modifier = Modifier, onSplashScreenRemoved: () -> Unit, ) { - LocalManagerProvider(LocalContext.current) { - LocalImageLoaderProvider(getDefaultImageLoader(LocalPlatformContext.current)) { + val context = LocalPlatformContext.current + val imageLoader = remember(context) { getDefaultImageLoader(context) } + LocalManagerProvider(LocalContext.current) { + LocalImageLoaderProvider(imageLoader) { ComposeApp(core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt-141-157 (1)
141-157:⚠️ Potential issue | 🟠 MajorBug:
loggeris assigned twice —httpLoggerparameter is silently overwritten.Line 142 sets
logger = httpLogger(the parameter), but line 152 immediately overwrites it with a Kermit-backed anonymousLogger. This means thehttpLoggerparameter (and its defaultLogger.DEFAULT) is dead code — callers cannot customize the logger.Pick one: either use the
httpLoggerparameter consistently, or remove it from the function signature and always use Kermit.🐛 Proposed fix — use `httpLogger` and remove the duplicate assignment
If the intent is to let callers customize the logger (with Kermit as the default):
fun setupDefaultHttpClient( ... - httpLogger: Logger = Logger.DEFAULT, + httpLogger: Logger = object : Logger { + override fun log(message: String) { + KermitLogger.d(tag = "KtorClient", messageString = message) + } + }, ... ): HttpClientConfig<*>.() -> Unit = { ... install(Logging) { logger = httpLogger level = httpLogLevel filter { request -> loggableHosts.any { host -> request.url.host.contains(host) } } sanitizeHeader { header -> header in sensitiveHeaders } - logger = object : Logger { - override fun log(message: String) { - KermitLogger.d(tag = "KtorClient", messageString = message) - } - } }core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt-144-148 (1)
144-148:⚠️ Potential issue | 🟠 MajorBug: default empty
loggableHostscauses the log filter to suppress all requests.
loggableHostsdefaults toemptyList()(line 75). The filter on line 144–147 uses.any { ... }which returnsfalsefor an empty list, so no HTTP traffic is ever logged unless the caller explicitly provides hosts. This is counterintuitive given thathttpLogLeveldefaults toLogLevel.ALL.Consider inverting the logic so that an empty
loggableHostsmeans "log everything" (no filtering).🐛 Proposed fix — treat empty list as "no filtering"
install(Logging) { logger = httpLogger level = httpLogLevel - filter { request -> - loggableHosts.any { host -> - request.url.host.contains(host) + if (loggableHosts.isNotEmpty()) { + filter { request -> + loggableHosts.any { host -> + request.url.host.contains(host) + } } }core-base/ui/src/commonMain/kotlin/template/core/base/ui/ImageLoaderExt.kt-53-65 (1)
53-65:⚠️ Potential issue | 🟠 Major
DebugLogger()is unconditionally enabled — avoid in release builds.
DebugLoggeremits verbose Coil internals on every image load. Leaving it active in production adds unnecessary allocations and log noise, and may expose internal implementation details. Guard it behind a debug check or remove it for production.Proposed fix
Since this is Kotlin Multiplatform, you can use an
expect/actualpattern or a build-config flag. A simple approach if you have aBuildConfigor similar mechanism:fun getDefaultImageLoader(context: PlatformContext): ImageLoader = ImageLoader .Builder(context) - .logger(DebugLogger()) + .apply { + if (isDebug) logger(DebugLogger()) + } .networkCachePolicy(CachePolicy.ENABLED)Alternatively, simply remove
.logger(DebugLogger())if no debug flag infrastructure exists yet, and re-add it only in debug/dev configurations.build-logic/convention/src/main/kotlin/org/mifos/mobile/KotlinMultiplatform.kt-30-31 (1)
30-31:⚠️ Potential issue | 🟠 MajorRemove both opt-in flags:
kotlin.RequiresOptInis incorrect, andkotlin.time.ExperimentalTimeis redundant.
- Line 30:
-opt-in=kotlin.RequiresOptInis incorrect and does nothing.kotlin.RequiresOptInis a meta-annotation used to declare other annotations as opt-in markers, not an experimental API marker itself. Remove this line entirely.- Line 31:
-opt-in=kotlin.time.ExperimentalTimeis redundant. The codebase already uses targeted@OptIn(ExperimentalTime::class)annotations at specific call sites whereClockand related APIs are used (e.g., LoanApplyViewModel, AccountsViewModel). The blanket project-wide flag is overly broad and should be removed in favor of the existing call-site opt-ins.♻️ Suggested diff
compilerOptions { freeCompilerArgs.add("-Xexpect-actual-classes") - freeCompilerArgs.add("-opt-in=kotlin.RequiresOptIn") - freeCompilerArgs.add("-opt-in=kotlin.time.ExperimentalTime") }cmp-android/dependencies/demoReleaseRuntimeClasspath.txt-303-304 (1)
303-304:⚠️ Potential issue | 🟠 MajorUpdate to stable FileKit version 0.12.0.
The FileKit Coil integration dependencies use version
0.10.0-beta04, a pre-release version unsuitable for release builds. Upgrade to the latest stable version0.12.0to ensure production stability.cmp-android/dependencies/prodReleaseRuntimeClasspath.txt-303-304 (1)
303-304:⚠️ Potential issue | 🟠 MajorBeta version in production release build.
The FileKit Coil dependencies at version
0.10.0-beta04are pre-release versions included in theprodReleasevariant that ships to end users. This version is declared centrally ingradle/libs.versions.tomland applied across all build variants (prod and demo, debug and release). Consider upgrading to a stable release or documenting the stability risk if pre-release features are required.core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/PerformanceTracker.kt-12-12 (1)
12-12:⚠️ Potential issue | 🟠 MajorAdd
@OptIn(ExperimentalTime::class)annotation to usekotlin.time.Clockin Kotlin 2.1.20.The code imports and uses
Clock.System.now().toEpochMilliseconds()(line 265), butkotlin.time.Clockremains experimental in Kotlin 2.1.20 and requires explicit opt-in. Add a file-level annotation before the package declaration or use@OptIn(ExperimentalTime::class)on the property to suppress the experimental API warning and enable compilation.core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/AnalyticsEvent.kt-258-277 (1)
258-277:⚠️ Potential issue | 🟠 MajorDocumentation-code mismatch: KDoc says
@throws IllegalArgumentExceptionbut the factory silently sanitizes.The KDoc at lines 222–257 states
@throws IllegalArgumentException if validation constraints are violatedand includes examples like// This would throw IllegalArgumentException (key too long). However, the actualinvokefactory (lines 268–277) silently truncates keys/values and falls back to"unknown_param"for blank keys—it never throws.The silent sanitization is a sensible choice for analytics (avoids crashing the app), but the KDoc contract must be updated to match.
Suggested KDoc fix (lines 222–257)
- * `@throws` IllegalArgumentException if validation constraints are violated + * Keys that are blank will be replaced with a fallback key ("unknown_param"). + * Keys exceeding 40 characters and values exceeding 100 characters are silently truncated.And remove/update the example comments:
- * // This would throw IllegalArgumentException (key too long) - * // val invalid = Param("this_key_is_way_too_long_and_exceeds_forty_characters", "value") + * // Key will be truncated to 40 characters + * // val param = Param("this_key_is_way_too_long_and_exceeds_forty_characters", "value")build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt-29-29 (1)
29-29:⚠️ Potential issue | 🟠 MajorAlign
targetSdkacross convention plugins —AndroidLibraryConventionPluginusestargetSdk = 34, whileAndroidApplicationConventionPlugin,KMPLibraryConventionPlugin, andKMPCoreBaseLibraryConventionPluginall usetargetSdk = 36. UpdateAndroidLibraryConventionPluginto match the others for consistency.core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt-28-33 (1)
28-33:⚠️ Potential issue | 🟠 Major
URLQueryAllowedCharacterSetdoes not escape&,=, or?in query values — subject/body containing these characters will break the mailto URI.
URLQueryAllowedCharacterSetpermits&,=, and?through unencoded because they're structurally valid in the query component. However, when encoding query values (subject, body), these characters must be escaped to avoid being misinterpreted as delimiters.For example, a subject of
"Q&A"would producemailto:to?subject=Q&A— parsingAas a new query parameter.Proposed fix — use a restricted character set for value encoding
private fun String.urlEncode(): String { - val nsString = this as NSString - return nsString.stringByAddingPercentEncodingWithAllowedCharacters( - NSCharacterSet.URLQueryAllowedCharacterSet, - ) ?: this + val allowed = NSCharacterSet.URLQueryAllowedCharacterSet.mutableCopy() as platform.Foundation.NSMutableCharacterSet + // Remove characters that are query-structure delimiters + allowed.removeCharactersInString("&=?+") + val nsString = this as NSString + return nsString.stringByAddingPercentEncodingWithAllowedCharacters(allowed) ?: this }Also applies to: 86-98
core/analytics/src/commonMain/kotlin/org/mifos/core/analytics/MifosAnalyticsEvent.kt-14-107 (1)
14-107: 🛠️ Refactor suggestion | 🟠 MajorWell-structured analytics constants, but currently unused.
The constants are well-organized by domain area and provide good coverage. However, as noted in the
MifosAnalyticsTracker.ktreview, the tracker uses hardcoded string literals instead of referencing these constants. Ensure the tracker and any other analytics call sites are updated to consumeMifosEventTypesandMifosParamKeysso this file fulfills its purpose.core-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.kt-108-116 (1)
108-116:⚠️ Potential issue | 🟠 MajorMissing
try-catcharoundstartActivitycalls —ActivityNotFoundExceptionwill crash the app.If no app can handle the intent (e.g., no browser for
openUrl, no dialer forcallPhone, no email client forsendEmail),startActivitythrowsActivityNotFoundException. The Desktop implementation wraps every call in try-catch, but the Android implementation does not. Consider wrapping eachstartActivitycall, or usingintent.resolveActivity(packageManager)to check first.Example fix for openUrl
actual fun openUrl(url: String) { val context = ShareUtils.activityProvider.invoke().application.baseContext - val uri = url.let { url.toUri() } + val uri = url.toUri() val intent = Intent(Intent.ACTION_VIEW).apply { data = uri addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) } - context.startActivity(intent) + try { + context.startActivity(intent) + } catch (e: Exception) { + android.util.Log.e("ShareUtils", "Error opening URL: ${e.message}") + } }Apply the same pattern to
openAppInfo,callPhone,sendEmail,sendViaSMS.Also applies to: 118-125, 127-135, 137-151, 153-166
core-base/ui/src/desktopMain/java/template/core/base/ui/ShareUtils.desktop.kt-71-84 (1)
71-84:⚠️ Potential issue | 🟠 Major
URLEncoder.encodeproduces+for spaces — mailto URIs expect%20.
java.net.URLEncoder.encodeis designed for HTML form encoding and encodes spaces as+. RFC 6068 (mailto) requires percent-encoding where spaces are%20. Recipients will see literal+characters instead of spaces in the subject/body.The same issue applies to
sendViaSMSon line 87.Proposed fix
- subject?.let { q.add("subject=" + java.net.URLEncoder.encode(it, Charsets.UTF_8)) } - body?.let { q.add("body=" + java.net.URLEncoder.encode(it, Charsets.UTF_8)) } + subject?.let { q.add("subject=" + java.net.URLEncoder.encode(it, Charsets.UTF_8).replace("+", "%20")) } + body?.let { q.add("body=" + java.net.URLEncoder.encode(it, Charsets.UTF_8).replace("+", "%20")) }Apply the same
.replace("+", "%20")tosendViaSMSat line 87.core/analytics/src/commonMain/kotlin/org/mifos/core/analytics/MifosAnalyticsTracker.kt-42-57 (1)
42-57: 🛠️ Refactor suggestion | 🟠 MajorHardcoded string literals instead of the constants defined in
MifosAnalyticsEvent.kt.This file uses inline strings like
"client_operation","client_id","loan_id", etc., whileMifosAnalyticsEvent.kt(added in this same PR) definesMifosEventTypesandMifosParamKeysconstants for exactly these purposes. OnlytrackLogin(line 31) references constants from the base module (Types,ParamKeys), making the pattern inconsistent even within this file.This applies to all track methods (lines 42–232). Using the centralized constants avoids typos and makes renaming/refactoring safe.
Example diff for trackClientOperation
fun trackClientOperation( operation: String, clientId: String? = null, success: Boolean = true, duration: Long? = null, ) { val params = mutableListOf( Param("client_operation", operation), Param(ParamKeys.SUCCESS, success.toString()), ) - clientId?.let { params.add(Param("client_id", it)) } + clientId?.let { params.add(Param(MifosParamKeys.CLIENT_ID, it)) } duration?.let { params.add(Param(ParamKeys.DURATION, "${it}ms")) } - analyticsHelper.logEvent(AnalyticsEvent("client_operation", params)) + analyticsHelper.logEvent(AnalyticsEvent(MifosEventTypes.CLIENT_PROFILE_VIEWED, params)) }core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt-36-36 (1)
36-36:⚠️ Potential issue | 🟠 MajorReplace deprecated
keyWindowAPI with scene-based window access for iOS 13+ compatibility.
UIApplication.sharedApplication().keyWindowwas deprecated in iOS 13 and will returnnilon devices with multiple scenes. Replace with:UIApplication.sharedApplication.connectedScenes .asSequence() .filterIsInstance<UIWindowScene>() .firstOrNull { it.activationState == UISceneActivationStateForegroundActive } ?.windows ?.firstOrNull { it.isKeyWindow } ?.rootViewControllerThis pattern appears at lines 36 and 123.
core/analytics/src/commonMain/kotlin/org/mifos/core/analytics/MifosAnalyticsExtensions.kt-121-132 (1)
121-132:⚠️ Potential issue | 🟠 MajorLogging preference old/new values could leak sensitive data.
trackPreferenceChangelogs botholdValueandnewValueinto analytics events without any filtering. If this is ever called for preferences like auth tokens, passcodes, or PII fields, those values will be sent to the analytics backend.Consider either documenting that callers must never pass sensitive values, or redacting/omitting values for sensitive preference keys.
core/analytics/src/commonMain/kotlin/org/mifos/core/analytics/MifosComposeAnalytics.kt-136-139 (1)
136-139:⚠️ Potential issue | 🟠 MajorDivision by zero when
totalStepsis 0.Line 138 computes
(step.toIntOrNull() ?: 0) * 100 / totalSteps. IftotalStepsis0, this throwsArithmeticException. Also,stepis typed asStringbut used for arithmetic — consider making itIntor at least guarding against zero.Proposed fix
- "progress_percentage" to "${(step.toIntOrNull() ?: 0) * 100 / totalSteps}", + "progress_percentage" to if (totalSteps > 0) { + "${(step.toIntOrNull() ?: 0) * 100 / totalSteps}" + } else { + "0" + },cmp-ios/Configuration/Config.xcconfig-3-3 (1)
3-3:⚠️ Potential issue | 🟠 MajorUpdate APP_NAME to match the rebranded display name.
APP_NAMEshould beLiteDoto matchINFOPLIST_KEY_CFBundleDisplayName, which is already set toLiteDoinproject.pbxproj. The current value ofMifos Mobileis inconsistent with the app's display name.scripts/pre-push.sh-30-33 (1)
30-33:⚠️ Potential issue | 🟠 Major
spotlessApplyfailure is silently ignored — false success message printed.If
spotlessApplyon Line 31 fails (non-zero exit), the script does not check its exit code and unconditionally prints the success message on Line 33. The user will believe formatting was fixed when it wasn't.Proposed fix
echo "🚀 Attempting to apply Spotless formatting fixes..." - ./gradlew spotlessApply --daemon > /tmp/spotless-result - rm /tmp/spotless-result - echo "🎉 Stellar job! Your code is pristine and has passed Spotless's formatting checks without a hitch! Keep shining bright! ✨🚀" + ./gradlew spotlessApply --daemon > /tmp/spotless-result + APPLY_EXIT_CODE=$? + rm /tmp/spotless-result + if [ ${APPLY_EXIT_CODE} -ne 0 ]; then + echo "💥 Spotless apply failed! Please fix the formatting issues manually." + exit ${APPLY_EXIT_CODE} + fi + echo "🎉 Stellar job! Your code is pristine and has passed Spotless's formatting checks without a hitch! Keep shining bright! ✨🚀"scripts/pre-push.sh-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorBranch guard no longer protects
development, contradicting project policy.The guard was changed from
"development"to"dev", but the project's primary integration branch isdevelopment(the PR target). This allows direct pushes todevelopmentto slip through unchecked. Based on learnings, the policy is "Always use feature branches; NEVER push directly todevelopmentbranch."Proposed fix
- if [ "$CURRENT_BRANCH" = "master" ] || [ "$CURRENT_BRANCH" = "dev" ]; then + if [ "$CURRENT_BRANCH" = "master" ] || [ "$CURRENT_BRANCH" = "development" ]; thenIf both
devanddevelopmentshould be protected:- if [ "$CURRENT_BRANCH" = "master" ] || [ "$CURRENT_BRANCH" = "dev" ]; then + if [ "$CURRENT_BRANCH" = "master" ] || [ "$CURRENT_BRANCH" = "dev" ] || [ "$CURRENT_BRANCH" = "development" ]; then.github/workflows/multi-platform-build-and-publish.yml-104-104 (1)
104-104:⚠️ Potential issue | 🟠 MajorIncompatible inputs for v1.0.8: the reusable workflow was updated to remove these inputs
The v1.0.8 tag exists, but the release notes specifically state that v1.0.8 "update[d] iOS configuration to read from
project_config.rb, removing redundant inputs" (PR#43). The inputsmetadata_path,use_cocoapods, andshared_moduleare likely no longer accepted by v1.0.8. Either remove these inputs from the workflow call or verify they are still valid for this version. Note: newer versions v1.0.9 and v1.0.10 are also available (released Feb 9, 2026).cmp-ios/iosApp.xcodeproj/project.pbxproj-385-385 (1)
385-385:⚠️ Potential issue | 🟠 MajorApp category changed from
financetoproductivity.Mifos is a financial inclusion platform. Changing
LSApplicationCategoryTypefrompublic.app-category.financetopublic.app-category.productivitywill mis-categorize the app in the App Store. This also looks like a template artifact.Also applies to: 419-419
cmp-ios/iosApp.xcodeproj/project.pbxproj-394-394 (1)
394-394:⚠️ Potential issue | 🟠 Major
PRODUCT_BUNDLE_IDENTIFIER[sdk=iphoneos*]set toorg.mifos.kmp.template— template placeholder.This sdk-conditional override will be used for on-device builds and App Store submissions. A bundle identifier containing "template" is almost certainly not the intended production identifier for mifos-mobile.
Also applies to: 428-428
cmp-ios/iosApp.xcodeproj/project.pbxproj-21-21 (1)
21-21:⚠️ Potential issue | 🟠 MajorProduct renamed to "LiteDo" — appears to be a template artifact, not appropriate for mifos-mobile.
The product name, display name, and
.appreferences have been changed from "Mifos Mobile" to "LiteDo". This repository isopenMF/mifos-mobile, so shipping an app named "LiteDo" looks like an accidental carry-over from the KMP template project. Please verify this is intentional.Also applies to: 72-72, 126-126, 384-384, 418-418
🟡 Minor comments (22)
cmp-ios/iosApp/ContentView.swift-16-17 (1)
16-17:⚠️ Potential issue | 🟡 MinorDuplicated comment fragment on line 17.
The inline comment reads
// .ignoresSafeArea(.keyboard) // Compose has own keyboard handler— the first// .ignoresSafeArea(.keyboard)portion is a stale copy-paste artifact. Clean it up to just the explanatory part.Proposed fix
.ignoresSafeArea(edges: .all) - .ignoresSafeArea(.keyboard) // .ignoresSafeArea(.keyboard) // Compose has own keyboard handler + .ignoresSafeArea(.keyboard) // Compose has own keyboard handlerfastlane/README.md-117-117 (1)
117-117:⚠️ Potential issue | 🟡 MinorCorrect the capitalization of "iOS".
"Ios" should be capitalized as "iOS" (the proper trademark for Apple's mobile operating system) in the descriptions on lines 117 and 125.
📝 Proposed fix for capitalization
-Build Ios application +Build iOS application-Build Signed Ios application +Build Signed iOS applicationNote: Since this README is auto-generated (line 169), you'll need to update the corresponding descriptions in your Fastfile source so the corrections persist when the README is regenerated.
Also applies to: 125-125
core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-40-46 (1)
40-46:⚠️ Potential issue | 🟡 MinorKDoc example doesn't match actual types.
The example shows
Result<List<User>, RemoteError>but the actual type isNetworkResult<List<User>, NetworkError>.Proposed fix
* ```kotlin * interface ApiService { * `@GET`("users") - * suspend fun getUsers(): Result<List<User>, RemoteError> + * suspend fun getUsers(): NetworkResult<List<User>, NetworkError> * } * ```core-base/network/src/commonMain/kotlin/template/core/base/network/NetworkError.kt-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorTypo in license header: duplicated "See".
See Seeshould beSee.This same typo is present in all three new files.
Proposed fix
- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEbuild-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt-32-36 (1)
32-36:⚠️ Potential issue | 🟡 MinorDuplicate dependency:
jb.lifecycle.composeis added twice.Line 32 and line 35 both add
jb.lifecycle.composetocommonMainImplementation. This is a copy-paste error — one of these should be removed, or line 35 was intended to be a different library (e.g.,jb.lifecycleSavedState?).Proposed fix — remove the duplicate
add("commonMainImplementation", libs.findLibrary("jb.composeViewmodel").get()) add("commonMainImplementation", libs.findLibrary("jb.lifecycleViewmodel").get()) - add("commonMainImplementation", libs.findLibrary("jb.lifecycle.compose").get()) add("commonMainImplementation", libs.findLibrary("jb.lifecycleViewmodelSavedState").get(),)core-base/platform/src/commonMain/kotlin/template/core/base/platform/LocalManagerProviders.kt-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorDuplicate word in license header: "See See"
Line 8 has a repeated "See" — should be just
See https://....- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEcmp-android/dependencies/demoReleaseRuntimeClasspath.txt-255-262 (1)
255-262:⚠️ Potential issue | 🟡 MinorUpdate dev.gitlive Firebase Kotlin SDK to v2.4.0; verify JS target dependency chain.
The
dev.gitlive:firebase-*packages at v2.1.0 are outdated. Latest available is v2.4.0 (released October 31, 2025). While no known security advisories exist for v2.1.0 itself, consider upgrading to 2.4.0 for bug fixes and maintenance currency.If the build targets JavaScript, confirm that the resolved
npm firebaseversion is ≥ 10.9.0 to mitigate upstream CVE-2024-11023. The firebase-kotlin-sdk project previously used npm firebase 9.19.1 (affected range) and updated this in PR#675.cmp-android/src/prod/AndroidManifest.xml-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorMinor: Copyright year inconsistency.
This new file uses "Copyright 2024" while other new files in this PR use "Copyright 2025" or "Copyright 2026". Consider updating to the current year for consistency.
cmp-android/src/main/kotlin/cmp/android/app/MainActivity.kt-12-14 (1)
12-14:⚠️ Potential issue | 🟡 Minor
ComponentActivityimport appears unused;@seereferences wrong class.The class extends
AppCompatActivity(line 40), butComponentActivityis imported (line 14) and referenced in the@seetag (line 37). The@Suppress("UnusedPrivateProperty")annotation on line 39 also seems unnecessary —userPreferencesRepositoryis actively used.Proposed fix
-import androidx.activity.ComponentActivity ... - * `@see` ComponentActivity + * `@see` AppCompatActivity */ -@Suppress("UnusedPrivateProperty") class MainActivity : AppCompatActivity() {Also applies to: 37-39
core-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.kt-110-110 (1)
110-110:⚠️ Potential issue | 🟡 MinorRedundant
.let— captures outerurlinstead ofit.
url.let { url.toUri() }ignores the lambda parameteritand references the outerurldirectly. The.letblock is dead code.Proposed fix
- val uri = url.let { url.toUri() } + val uri = url.toUri().github/workflows/cache-cleanup.yaml-12-12 (1)
12-12:⚠️ Potential issue | 🟡 Minor
cleanup_prwill always befalsefor this public repository.The condition
github.event.repository.private == truemeans the PR cache cleanup will never trigger foropenMF/mifos-mobile, which is a public repo. This was likely copied from a template designed for private repositories.If PR-scoped cache cleanup is intended, remove the private-repo guard:
Proposed fix
- cleanup_pr: ${{ github.event_name == 'pull_request' && github.event.repository.private == true }} + cleanup_pr: ${{ github.event_name == 'pull_request' }}scripts/deploy_testflight.sh-152-154 (1)
152-154:⚠️ Potential issue | 🟡 MinorContact PII (email, name, phone) printed to stdout — may leak into CI logs.
These environment variables may contain personal contact information. If this script is ever executed in CI, the values will appear in build logs. Consider masking or omitting them from output.
scripts/pre-push.sh-60-61 (1)
60-61:⚠️ Potential issue | 🟡 MinorMisleading comment: says "ktlint checks" but function runs dependency guard.
Proposed fix
-# Function to run ktlint checks +# Function to run dependency guard checksscripts/deploy_firebase.sh-119-119 (1)
119-119:⚠️ Potential issue | 🟡 MinorDeclare and assign
MATCH_PASSWORDseparately (SC2155) — same as flagged indeploy_appstore.sh.Proposed fix
-export MATCH_PASSWORD=$(cat secrets/.match_password) +MATCH_PASSWORD=$(cat secrets/.match_password) +export MATCH_PASSWORDscripts/deploy_testflight.sh-126-126 (1)
126-126:⚠️ Potential issue | 🟡 MinorDeclare and assign
MATCH_PASSWORDseparately (SC2155).Same issue as the other deploy scripts.
Proposed fix
-export MATCH_PASSWORD=$(cat secrets/.match_password) +MATCH_PASSWORD=$(cat secrets/.match_password) +export MATCH_PASSWORD.github/workflows/pr-check.yml-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorDocumentation says Java 17 but workflow configures Java 21.
Lines 16 and 39 in the comments both state "Java 17" as a prerequisite, but Line 94 sets
java-version: '21'. Update the comments to match the actual configuration, or correct the input — whichever is intended.📝 Proposed fix to align comments with config
-# - Sets up Java 17 +# - Sets up Java 21-# - Java 17 +# - Java 21Also applies to: 39-39, 94-94
scripts/deploy_appstore.sh-131-131 (1)
131-131:⚠️ Potential issue | 🟡 MinorDeclare and assign separately to avoid masking the return value of
cat.If
cat secrets/.match_passwordfails, the error is masked becauseexportalways succeeds. This was also flagged by ShellCheck (SC2155). The same issue exists indeploy_firebase.sh(Line 119) anddeploy_testflight.sh(Line 126).Proposed fix
-export MATCH_PASSWORD=$(cat secrets/.match_password) +MATCH_PASSWORD=$(cat secrets/.match_password) +export MATCH_PASSWORDscripts/setup_apn_key.sh-111-111 (1)
111-111:⚠️ Potential issue | 🟡 MinorArray assignment via command substitution splits on whitespace — breaks on paths with spaces.
P8_FILES=($(find ...))will incorrectly split filenames containing spaces. ShellCheck also flags this (SC2207).Proposed fix using `mapfile`
-P8_FILES=($(find . -maxdepth 2 -name "AuthKey_*.p8" 2>/dev/null | grep -v "secrets/AuthKey.p8")) +mapfile -t P8_FILES < <(find . -maxdepth 2 -name "AuthKey_*.p8" 2>/dev/null | grep -v "secrets/AuthKey.p8")cmp-ios/iosApp/Info.plist-27-32 (1)
27-32:⚠️ Potential issue | 🟡 MinorPrivacy usage descriptions stating "does not use" the feature may face App Store review scrutiny.
Apple may question why these keys are declared if the app doesn't use the features. If these are present because a linked framework references the APIs, consider rewording to something like "Required by an integrated SDK" rather than "This message is required for compliance only." If the features are genuinely unused and no framework references them, removing the keys entirely would be cleaner.
cmp-ios/iosApp/Info.plist-33-34 (1)
33-34:⚠️ Potential issue | 🟡 Minor
NSPhotoLibraryAddUsageDescriptionreferences "artworks" — likely a leftover from the KMP template.This is a financial services app (Mifos Mobile), so "save artworks directly to your device and view them offline" doesn't match the app's purpose. Update this to reflect actual usage (e.g., saving receipts, documents, or profile photos).
Proposed fix
<key>NSPhotoLibraryAddUsageDescription</key> -<string>Allow access to add photos to your library so you can save artworks directly to your device and view them offline.</string> +<string>Allow access to add photos to your library so you can save documents and receipts to your device.</string>.github/workflows/tag-weekly-release.yml-38-40 (1)
38-40:⚠️ Potential issue | 🟡 MinorComment says default branch is
'dev', but the workflow uses'development'.Line 39 states the default target branch is
'dev', but the actualrefon Line 110 is'development'. This could confuse future maintainers.Proposed fix
# Workflow Inputs: -# - target_branch: Branch to use for releases (default: 'dev') +# - target_branch: Branch to use for releases (default: 'development') # Allows flexible release targeting across different branchesscripts/setup_ios_complete.sh-208-208 (1)
208-208:⚠️ Potential issue | 🟡 MinorUse
mapfileinstead of array assignment from command substitution to handle filenames with spaces.Static analysis (SC2207) flags this line. If any
.p8file path contains spaces, the array will be incorrectly split.Proposed fix
-P8_FILES=($(find . -maxdepth 2 -name "AuthKey_*.p8" -o -name "*.p8" 2>/dev/null)) +mapfile -t P8_FILES < <(find . -maxdepth 2 \( -name "AuthKey_*.p8" -o -name "*.p8" \) 2>/dev/null)Note: the
findalso needs grouping\( ... \)around the-oto ensure both name patterns apply the-maxdepth.
| extensions.configure<LibraryExtension> { | ||
| configureKotlinAndroid(this) | ||
| defaultConfig.targetSdk = 36 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and display KotlinAndroid.kt content
fd "KotlinAndroid.kt" --exec cat -n {}Repository: openMF/mifos-mobile
Length of output: 3110
🏁 Script executed:
# Search for all compileSdk and targetSdk occurrences with context
rg -n 'compileSdk|targetSdk' --type=kt -C 2Repository: openMF/mifos-mobile
Length of output: 89
🏁 Script executed:
# Search for targetSdk across all Kotlin files
rg -n 'targetSdk' -A 2 -B 2Repository: openMF/mifos-mobile
Length of output: 3153
🏁 Script executed:
# Find and display KMPCoreBaseLibraryConventionPlugin.kt
fd "KMPCoreBaseLibraryConventionPlugin.kt" --exec cat -n {}Repository: openMF/mifos-mobile
Length of output: 2342
targetSdk = 36 exceeds compileSdk = 35 — violates Android platform requirements.
configureKotlinAndroid(this) sets compileSdk = 35 (line 30), but the next line sets targetSdk = 36. Android requires targetSdk ≤ compileSdk; targeting a higher API level than the compilation target is unsupported and violates platform best practices.
Align targetSdk to 35 or bump compileSdk to 36 in KotlinAndroid.kt.
🤖 Prompt for AI Agents
In `@build-logic/convention/src/main/kotlin/KMPCoreBaseLibraryConventionPlugin.kt`
around lines 29 - 31, The PR sets defaultConfig.targetSdk = 36 while
configureKotlinAndroid(...) configures compileSdk = 35, which violates Android's
requirement targetSdk ≤ compileSdk; either set defaultConfig.targetSdk to 35
here or update the compileSdk in KotlinAndroid.kt to 36. Locate the call site
configureKotlinAndroid in KMPCoreBaseLibraryConventionPlugin.kt and change
defaultConfig.targetSdk to 35 (or alternatively bump the compileSdk
constant/value in KotlinAndroid.kt to 36) so targetSdk and compileSdk are
consistent.
| // State to trigger recomposition when locale changes | ||
| var localeVersion by remember { mutableStateOf(0) } | ||
|
|
||
| // Use key() to force complete recomposition when locale changes | ||
| key(localeVersion) { | ||
| SharedApp( | ||
| handleThemeMode = {}, | ||
| handleAppLocale = { languageTag -> | ||
| if (languageTag != null) { | ||
| // Store language preference in localStorage | ||
| localStorage.setItem("app_language", languageTag) | ||
| // Set HTML lang attribute for accessibility | ||
| document.documentElement?.setAttribute("lang", languageTag) | ||
| } else { | ||
| // System Default: remove stored language preference | ||
| localStorage.removeItem("app_language") | ||
| // Reset to browser's default language | ||
| val browserLang = window.navigator.language | ||
| document.documentElement?.setAttribute("lang", browserLang) | ||
| } | ||
| // Reload page to apply language changes (required for web) | ||
| // Note: This will reload the page, and locale selection depends on browser settings | ||
| // window.location.reload() | ||
| }, | ||
| onSplashScreenRemoved = {} | ||
| ) | ||
| } |
There was a problem hiding this comment.
Bug: localeVersion is never incremented — locale changes won't trigger recomposition.
The localeVersion state drives the key() block, but the handleAppLocale callback never increments it. Compare with cmp-desktop/src/jvmMain/kotlin/main.kt (Line 76) which correctly calls localeVersion++. Without this, locale changes update localStorage and the HTML attribute but the Compose tree won't recompose.
🐛 Proposed fix
val browserLang = window.navigator.language
document.documentElement?.setAttribute("lang", browserLang)
}
- // Reload page to apply language changes (required for web)
- // Note: This will reload the page, and locale selection depends on browser settings
- // window.location.reload()
+ localeVersion++
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // State to trigger recomposition when locale changes | |
| var localeVersion by remember { mutableStateOf(0) } | |
| // Use key() to force complete recomposition when locale changes | |
| key(localeVersion) { | |
| SharedApp( | |
| handleThemeMode = {}, | |
| handleAppLocale = { languageTag -> | |
| if (languageTag != null) { | |
| // Store language preference in localStorage | |
| localStorage.setItem("app_language", languageTag) | |
| // Set HTML lang attribute for accessibility | |
| document.documentElement?.setAttribute("lang", languageTag) | |
| } else { | |
| // System Default: remove stored language preference | |
| localStorage.removeItem("app_language") | |
| // Reset to browser's default language | |
| val browserLang = window.navigator.language | |
| document.documentElement?.setAttribute("lang", browserLang) | |
| } | |
| // Reload page to apply language changes (required for web) | |
| // Note: This will reload the page, and locale selection depends on browser settings | |
| // window.location.reload() | |
| }, | |
| onSplashScreenRemoved = {} | |
| ) | |
| } | |
| // State to trigger recomposition when locale changes | |
| var localeVersion by remember { mutableStateOf(0) } | |
| // Use key() to force complete recomposition when locale changes | |
| key(localeVersion) { | |
| SharedApp( | |
| handleThemeMode = {}, | |
| handleAppLocale = { languageTag -> | |
| if (languageTag != null) { | |
| // Store language preference in localStorage | |
| localStorage.setItem("app_language", languageTag) | |
| // Set HTML lang attribute for accessibility | |
| document.documentElement?.setAttribute("lang", languageTag) | |
| } else { | |
| // System Default: remove stored language preference | |
| localStorage.removeItem("app_language") | |
| // Reset to browser's default language | |
| val browserLang = window.navigator.language | |
| document.documentElement?.setAttribute("lang", browserLang) | |
| } | |
| localeVersion++ | |
| }, | |
| onSplashScreenRemoved = {} | |
| ) | |
| } |
🤖 Prompt for AI Agents
In `@cmp-web/src/jsMain/kotlin/Application.kt` around lines 36 - 62, The
localeVersion state (var localeVersion by remember { mutableStateOf(0) }) used
as the key for the key(localeVersion) block is never incremented, so changing
locale in handleAppLocale doesn't trigger recomposition; update the
handleAppLocale lambda in Application.kt (the one passed to SharedApp) to
increment localeVersion (localeVersion++) after you modify localStorage and
document.documentElement attributes (do this in both the non-null languageTag
branch and the else branch) so key(localeVersion) forces a full recomposition
when the locale changes.
| // State to trigger recomposition when locale changes | ||
| var localeVersion by remember { mutableStateOf(0) } | ||
|
|
||
| // Use key() to force complete recomposition when locale changes | ||
| key(localeVersion) { | ||
| /* | ||
| * Invokes the root composable of the application. | ||
| * This function is responsible for setting up the entire UI structure of the app. | ||
| */ | ||
| SharedApp( | ||
| handleThemeMode = {}, | ||
| handleAppLocale = { languageTag -> | ||
| if (languageTag != null) { | ||
| // Store language preference in localStorage | ||
| localStorage.setItem("app_language", languageTag) | ||
| // Set HTML lang attribute for accessibility | ||
| document.documentElement?.setAttribute("lang", languageTag) | ||
| } else { | ||
| // System Default: remove stored language preference | ||
| localStorage.removeItem("app_language") | ||
| // Reset to browser's default language | ||
| val browserLang = window.navigator.language | ||
| document.documentElement?.setAttribute("lang", browserLang) | ||
| } | ||
| // Reload page to apply language changes (required for web) | ||
| // Note: This will reload the page, and locale selection depends on browser settings | ||
| // window.location.reload() | ||
| }, | ||
| onSplashScreenRemoved = {}, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Same bug: localeVersion is never incremented — locale recomposition is broken.
Same issue as in cmp-web/src/jsMain/kotlin/Application.kt. Add localeVersion++ at the end of the handleAppLocale callback to trigger the key()-based recomposition.
🐛 Proposed fix
val browserLang = window.navigator.language
document.documentElement?.setAttribute("lang", browserLang)
}
- // Reload page to apply language changes (required for web)
- // Note: This will reload the page, and locale selection depends on browser settings
- // window.location.reload()
+ localeVersion++
},🤖 Prompt for AI Agents
In `@cmp-web/src/wasmJsMain/kotlin/Main.kt` around lines 57 - 87, The issue is
that localeVersion (used with key(localeVersion) to force recomposition) is
never incremented so locale changes don't trigger recomposition; inside the
handleAppLocale lambda passed to SharedApp, after updating localStorage and
document.documentElement (and after the else branch actions), increment
localeVersion (i.e., localeVersion++ or localeVersion = localeVersion + 1) so
key(localeVersion) will change and force a full recomposition; locate the
localeVersion variable declaration and the SharedApp(handleAppLocale = { ... })
callback to make this change.
| package template.core.base.analytics | ||
|
|
||
| import kotlinx.datetime.Clock | ||
| import kotlin.time.Clock |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
kotlin.time.Clock kotlin.time.Instant toEpochMilliseconds Kotlin version introduced
💡 Result:
kotlin.time.Clock/kotlin.time.Instant: first added to the Kotlin standard library in Kotlin 2.1.20 (as Experimental time-tracking functionality). [1]kotlin.time.Instant.toEpochMilliseconds(): marked “Since Kotlin 2.3” (i.e., introduced/available starting in Kotlin 2.3). [2]- Same for
Instant.fromEpochMilliseconds(...): Since Kotlin 2.3. [3]
- Same for
- Current stdlib API docs also list
kotlin.time.Clockas “Since Kotlin 2.3”. [4]
Sources: [1] [2] [3] [4]
🏁 Script executed:
# Find and inspect the AnalyticsExtensions.kt file
find . -type f -name "AnalyticsExtensions.kt" | head -5Repository: openMF/mifos-mobile
Length of output: 158
🏁 Script executed:
# Read the AnalyticsExtensions.kt file
cat -n core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/AnalyticsExtensions.ktRepository: openMF/mifos-mobile
Length of output: 8279
🏁 Script executed:
# Find build.gradle.kts files to check Kotlin version
find . -type f -name "build.gradle.kts" | head -3Repository: openMF/mifos-mobile
Length of output: 154
🏁 Script executed:
# Check the main build.gradle.kts for Kotlin version
head -100 build.gradle.kts | grep -i kotlinRepository: openMF/mifos-mobile
Length of output: 378
🏁 Script executed:
# Also check build-logic for plugin versions
head -150 build-logic/convention/build.gradle.kts | grep -i kotlinRepository: openMF/mifos-mobile
Length of output: 244
🏁 Script executed:
# Find and check gradle/libs.versions.toml or libs.versions.toml
find . -name "libs.versions.toml" -o -name "gradle.properties" | head -5Repository: openMF/mifos-mobile
Length of output: 142
🏁 Script executed:
# Check libs.versions.toml for Kotlin version
cat gradle/libs.versions.toml | grep -A 5 -B 5 "kotlin"Repository: openMF/mifos-mobile
Length of output: 7952
kotlin.time.Instant.toEpochMilliseconds() is not available in Kotlin 2.1.20.
The project targets Kotlin 2.1.20, but toEpochMilliseconds() was only added in Kotlin 2.3. The code at lines 118, 121, 145, 148, and 157 will fail to compile. Either upgrade the project to Kotlin 2.3 or later, or revert to kotlinx.datetime.Clock.
Additionally, if staying on Kotlin 2.1.20, kotlin.time.Clock may require an @OptIn(ExperimentalTime::class) annotation since it was marked experimental when first introduced.
🤖 Prompt for AI Agents
In
`@core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/AnalyticsExtensions.kt`
at line 12, Replace the usage of kotlin.time.Clock and
kotlin.time.Instant.toEpochMilliseconds (which are not available in Kotlin
2.1.20) with the stable kotlinx.datetime equivalents: use kotlinx.datetime.Clock
and kotlinx.datetime.Instant.toEpochMilliseconds(), or alternatively upgrade the
project Kotlin version to 2.3+ so kotlin.time APIs are available; if you choose
to keep kotlin.time on older compiler, add `@OptIn`(ExperimentalTime::class) to
the relevant functions/classes (e.g., in AnalyticsExtensions.kt) to suppress
experimental API errors—pick one approach and apply it consistently to all sites
that call toEpochMilliseconds (lines referencing Instant.toEpochMilliseconds and
imports of kotlin.time.Clock).
| in 200..209 -> { | ||
| try { | ||
| val data = result.response.body(successType) as Any | ||
| NetworkResult.Success(data) | ||
| } catch (e: NoTransformationFoundException) { | ||
| NetworkResult.Error(NetworkError.SERIALIZATION) | ||
| } catch (e: SerializationException) { | ||
| println("Serialization error: ${e.message}") | ||
| NetworkResult.Error(NetworkError.SERIALIZATION) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Success range 200..209 should be 200..299.
HTTP 2xx status codes span 200–299. The range 200..209 will misclassify valid success responses like 226 IM Used (rare, but the standard range matters). More critically, this is almost certainly a typo for 200..299, since every HTTP library and specification defines 2xx as 200–299.
Additionally, a 204 No Content response has no body — calling body() on it will throw, and it will be misreported as a SERIALIZATION error rather than a success. Consider handling 204 explicitly if your APIs may return it.
Proposed fix
- in 200..209 -> {
+ in 200..299 -> {If you need to handle 204 No Content gracefully:
+ 204 -> {
+ NetworkResult.Success(Unit)
+ }
+
- in 200..209 -> {
+ in 200..299 -> {🤖 Prompt for AI Agents
In
`@core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt`
around lines 88 - 98, In ResultSuspendConverterFactory update the success-range
check from 200..209 to 200..299 and handle 204 No Content before calling
result.response.body(successType): if status is 204, return
NetworkResult.Success with an appropriate empty/null value for successType
instead of invoking body() (which triggers
NoTransformationFoundException/SerializationException and yields
NetworkError.SERIALIZATION); otherwise call result.response.body(successType)
and wrap in NetworkResult.Success, preserving the existing catches for
NoTransformationFoundException and SerializationException that return
NetworkResult.Error(NetworkError.SERIALIZATION).
| eval $FASTLANE_CMD | ||
|
|
||
| # Check if deployment was successful | ||
| if [ $? -eq 0 ]; then | ||
| print_section "✅ Deployment Successful!" | ||
| print_success "Your iOS app has been uploaded to App Store Connect" | ||
|
|
||
| if [ "$SUBMIT_FOR_REVIEW" != "false" ]; then | ||
| print_info "Next steps:" | ||
| echo " 1. Monitor review status in App Store Connect" | ||
| echo " 2. Check https://appstoreconnect.apple.com/" | ||
| echo " 3. Go to App Store → iOS App → App Store" | ||
| echo " 4. Watch for status changes:" | ||
| echo " - Processing → Waiting for Review → In Review → Approved/Rejected" | ||
| echo | ||
| print_warning "Review typically takes 24-72 hours" | ||
|
|
||
| if [ "$AUTOMATIC_RELEASE" != "false" ]; then | ||
| print_warning "App will go live AUTOMATICALLY after approval!" | ||
| else | ||
| print_info "You'll need to manually release the app after approval" | ||
| fi | ||
| else | ||
| print_info "Binary uploaded but NOT submitted for review" | ||
| print_info "Go to App Store Connect to manually submit when ready" | ||
| fi | ||
|
|
||
| echo | ||
| print_info "Useful links:" | ||
| echo " - App Store Connect: https://appstoreconnect.apple.com/" | ||
| echo " - App Store Review Guidelines: https://developer.apple.com/app-store/review/guidelines/" | ||
| echo " - Common Rejection Reasons: https://developer.apple.com/app-store/review/" | ||
| else | ||
| print_section "❌ Deployment Failed" | ||
| print_error "Please check the error messages above" | ||
| print_info "Common issues:" | ||
| echo " - Invalid Match password" | ||
| echo " - SSH key not added to Match repository" | ||
| echo " - Invalid App Store Connect API key" | ||
| echo " - Certificate/provisioning profile issues" | ||
| echo " - Build number conflicts (already uploaded)" | ||
| echo " - Missing app metadata in App Store Connect" | ||
| echo " - Missing required assets (screenshots, app icon, etc.)" | ||
| echo " - Export compliance information missing" | ||
| exit 1 |
There was a problem hiding this comment.
set -e makes the failure-handling branch (Lines 299-310) unreachable dead code.
set -e (Line 9) causes the script to exit immediately when eval $FASTLANE_CMD returns non-zero. The if [ $? -eq 0 ] on Line 269 will therefore never see a failure — the script will have already exited. The same pattern exists in deploy_firebase.sh (Lines 150/153) and deploy_testflight.sh (Lines 182/185).
Fix by either disabling set -e before the command or using || true to capture the exit code:
Proposed fix
-eval $FASTLANE_CMD
-
-# Check if deployment was successful
-if [ $? -eq 0 ]; then
+DEPLOY_EXIT_CODE=0
+eval $FASTLANE_CMD || DEPLOY_EXIT_CODE=$?
+
+# Check if deployment was successful
+if [ $DEPLOY_EXIT_CODE -eq 0 ]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eval $FASTLANE_CMD | |
| # Check if deployment was successful | |
| if [ $? -eq 0 ]; then | |
| print_section "✅ Deployment Successful!" | |
| print_success "Your iOS app has been uploaded to App Store Connect" | |
| if [ "$SUBMIT_FOR_REVIEW" != "false" ]; then | |
| print_info "Next steps:" | |
| echo " 1. Monitor review status in App Store Connect" | |
| echo " 2. Check https://appstoreconnect.apple.com/" | |
| echo " 3. Go to App Store → iOS App → App Store" | |
| echo " 4. Watch for status changes:" | |
| echo " - Processing → Waiting for Review → In Review → Approved/Rejected" | |
| echo | |
| print_warning "Review typically takes 24-72 hours" | |
| if [ "$AUTOMATIC_RELEASE" != "false" ]; then | |
| print_warning "App will go live AUTOMATICALLY after approval!" | |
| else | |
| print_info "You'll need to manually release the app after approval" | |
| fi | |
| else | |
| print_info "Binary uploaded but NOT submitted for review" | |
| print_info "Go to App Store Connect to manually submit when ready" | |
| fi | |
| echo | |
| print_info "Useful links:" | |
| echo " - App Store Connect: https://appstoreconnect.apple.com/" | |
| echo " - App Store Review Guidelines: https://developer.apple.com/app-store/review/guidelines/" | |
| echo " - Common Rejection Reasons: https://developer.apple.com/app-store/review/" | |
| else | |
| print_section "❌ Deployment Failed" | |
| print_error "Please check the error messages above" | |
| print_info "Common issues:" | |
| echo " - Invalid Match password" | |
| echo " - SSH key not added to Match repository" | |
| echo " - Invalid App Store Connect API key" | |
| echo " - Certificate/provisioning profile issues" | |
| echo " - Build number conflicts (already uploaded)" | |
| echo " - Missing app metadata in App Store Connect" | |
| echo " - Missing required assets (screenshots, app icon, etc.)" | |
| echo " - Export compliance information missing" | |
| exit 1 | |
| DEPLOY_EXIT_CODE=0 | |
| eval $FASTLANE_CMD || DEPLOY_EXIT_CODE=$? | |
| # Check if deployment was successful | |
| if [ $DEPLOY_EXIT_CODE -eq 0 ]; then | |
| print_section "✅ Deployment Successful!" | |
| print_success "Your iOS app has been uploaded to App Store Connect" | |
| if [ "$SUBMIT_FOR_REVIEW" != "false" ]; then | |
| print_info "Next steps:" | |
| echo " 1. Monitor review status in App Store Connect" | |
| echo " 2. Check https://appstoreconnect.apple.com/" | |
| echo " 3. Go to App Store → iOS App → App Store" | |
| echo " 4. Watch for status changes:" | |
| echo " - Processing → Waiting for Review → In Review → Approved/Rejected" | |
| echo | |
| print_warning "Review typically takes 24-72 hours" | |
| if [ "$AUTOMATIC_RELEASE" != "false" ]; then | |
| print_warning "App will go live AUTOMATICALLY after approval!" | |
| else | |
| print_info "You'll need to manually release the app after approval" | |
| fi | |
| else | |
| print_info "Binary uploaded but NOT submitted for review" | |
| print_info "Go to App Store Connect to manually submit when ready" | |
| fi | |
| echo | |
| print_info "Useful links:" | |
| echo " - App Store Connect: https://appstoreconnect.apple.com/" | |
| echo " - App Store Review Guidelines: https://developer.apple.com/app-store/review/guidelines/" | |
| echo " - Common Rejection Reasons: https://developer.apple.com/app-store/review/" | |
| else | |
| print_section "❌ Deployment Failed" | |
| print_error "Please check the error messages above" | |
| print_info "Common issues:" | |
| echo " - Invalid Match password" | |
| echo " - SSH key not added to Match repository" | |
| echo " - Invalid App Store Connect API key" | |
| echo " - Certificate/provisioning profile issues" | |
| echo " - Build number conflicts (already uploaded)" | |
| echo " - Missing app metadata in App Store Connect" | |
| echo " - Missing required assets (screenshots, app icon, etc.)" | |
| echo " - Export compliance information missing" | |
| exit 1 |
🤖 Prompt for AI Agents
In `@scripts/deploy_appstore.sh` around lines 266 - 310, The script uses set -e
which causes eval $FASTLANE_CMD to exit the script on failure, making the
subsequent if [ $? -eq 0 ] branch unreachable; change the flow so you capture
the fastlane exit code instead of allowing set -e to terminate execution: either
temporarily disable set -e before eval $FASTLANE_CMD and re-enable it after, or
run eval $FASTLANE_CMD in a way that prevents immediate exit (e.g., append a
no-fail guard) and store its exit status into a variable (e.g.,
FASTLANE_EXIT_CODE) then replace references to $? in the success/failure checks
with that variable; target the eval $FASTLANE_CMD invocation and the if [ $? -eq
0 ] conditional in deploy_appstore.sh (and apply the same change to the
analogous locations in deploy_firebase.sh and deploy_testflight.sh).
| bundle exec fastlane ios deploy_on_firebase | ||
|
|
||
| # Check if deployment was successful | ||
| if [ $? -eq 0 ]; then | ||
| print_section "✅ Deployment Successful!" | ||
| print_success "Your iOS app has been deployed to Firebase App Distribution" | ||
| print_info "Testers will receive a notification with download instructions" | ||
| else | ||
| print_section "❌ Deployment Failed" | ||
| print_error "Please check the error messages above" | ||
| print_info "Common issues:" | ||
| echo " - Invalid Match password" | ||
| echo " - SSH key not added to Match repository" | ||
| echo " - Invalid Firebase credentials" | ||
| echo " - Certificate/provisioning profile issues" | ||
| exit 1 |
There was a problem hiding this comment.
Same set -e + $? dead-code issue as in deploy_appstore.sh — failure block is unreachable.
set -e (Line 9) will terminate the script if bundle exec fastlane on Line 150 fails, so the else branch on Line 157 is never reached. Apply the same fix pattern described in the deploy_appstore.sh review.
🤖 Prompt for AI Agents
In `@scripts/deploy_firebase.sh` around lines 150 - 165, Because the script uses
set -e, the if/else that checks "$?" after running "bundle exec fastlane ios
deploy_on_firebase" is dead code; change to capture the fastlane exit code
immediately (e.g., run the command, assign its exit status to a variable like
deploy_exit=$?), then use that variable in the conditional instead of relying on
"$?" later. Update the conditional that references "$?" to check the saved
variable (e.g., deploy_exit) and keep the same success and failure branches
(print_section/print_success/print_error/print_info and exit 1) so the failure
branch is reachable despite set -e.
| bundle exec fastlane ios beta | ||
|
|
||
| # Check if deployment was successful | ||
| if [ $? -eq 0 ]; then | ||
| print_section "✅ Deployment Successful!" | ||
| print_success "Your iOS app has been uploaded to TestFlight" | ||
| print_info "Next steps:" | ||
| echo " 1. Monitor beta review status in App Store Connect" | ||
| echo " 2. Check https://appstoreconnect.apple.com/" | ||
| echo " 3. Go to TestFlight → iOS → Builds" | ||
| echo " 4. Wait for 'Ready to Submit' → 'Waiting for Review' → 'In Review' → 'Approved'" | ||
| echo " 5. Once approved, testers will receive notifications" | ||
| echo | ||
| print_info "Beta review typically takes 24-48 hours" | ||
| else | ||
| print_section "❌ Deployment Failed" | ||
| print_error "Please check the error messages above" | ||
| print_info "Common issues:" | ||
| echo " - Invalid Match password" | ||
| echo " - SSH key not added to Match repository" | ||
| echo " - Invalid App Store Connect API key" | ||
| echo " - Certificate/provisioning profile issues" | ||
| echo " - Build number conflicts (already uploaded)" | ||
| echo " - Missing beta review information" | ||
| exit 1 |
There was a problem hiding this comment.
Same set -e + $? dead-code issue — failure block unreachable.
Identical to the issue flagged in deploy_appstore.sh and deploy_firebase.sh. The else branch (Line 197) is never executed because set -e terminates the script on Line 182 failure.
🤖 Prompt for AI Agents
In `@scripts/deploy_testflight.sh` around lines 182 - 206, The failure branch is
unreachable due to set -e causing the script to exit on a failing command;
change the invocation so the script can test its result instead of relying on $?
after a failing command. Replace the plain "bundle exec fastlane ios beta" +
subsequent "$? -eq 0" check with a conditional run like "if bundle exec fastlane
ios beta; then" ... "else" ... "fi" (so the else block with
print_section/print_error/print_info runs on failure) or alternatively run the
command and capture its exit code explicitly (e.g., "bundle exec fastlane ios
beta || exit_code=$?") before checking exit_code; update uses of print_section,
print_success, print_error accordingly.
| # This script verifies that your APN configuration is correct | ||
| # ============================================================================== | ||
|
|
||
| set -e # Exit on any error |
There was a problem hiding this comment.
set -e will abort the script on the first ((ERRORS++)) or ((WARNINGS++)) when the counter is 0.
In bash, ((expr)) returns exit code 1 when the expression evaluates to 0. Since ERRORS starts at 0, the post-increment ((ERRORS++)) evaluates to 0 (the old value), triggering set -e to terminate the script immediately on the first error. Same for WARNINGS.
Proposed fix — use arithmetic that never evaluates to zero
-ERRORS=0
-WARNINGS=0
+ERRORS=0
+WARNINGS=0
# ... throughout the script, replace all occurrences:
- ((ERRORS++))
+ ERRORS=$((ERRORS + 1))
- ((WARNINGS++))
+ WARNINGS=$((WARNINGS + 1))Alternatively, keep (( )) but guard it: ((++ERRORS)) (pre-increment, always ≥1) or ((ERRORS += 1)) || true.
Also applies to: 52-53, 73-73
🤖 Prompt for AI Agents
In `@scripts/verify_apn_setup.sh` at line 9, The script uses post-increment
arithmetic like ((ERRORS++)) and ((WARNINGS++)), which returns exit code 1 when
the old value is 0 and causes set -e to abort; replace those occurrences (the
((ERRORS++)) and ((WARNINGS++)) expressions) with a non-zero-evaluating
increment such as ((++ERRORS)) and ((++WARNINGS)) (or alternately
ERRORS=$((ERRORS+1)) / WARNINGS=$((WARNINGS+1)) or ((ERRORS+=1)) || true) so
increments never evaluate to 0 and set -e will not prematurely exit.
| # This script performs comprehensive verification of iOS deployment configuration | ||
| # ============================================================================== | ||
|
|
||
| set -e |
There was a problem hiding this comment.
set -e will abort the script on the first failed check(), breaking the summary logic.
set -e causes the script to exit whenever a command (including a function call) returns non-zero. check() returns 1 on failure (line 65), so the very first failing check will terminate the script before any subsequent checks run or the summary (lines 329–364) is reached.
This defeats the purpose of accumulating PASSED_CHECKS/FAILED_CHECKS/WARNING_CHECKS and printing a consolidated report.
Fix: either remove set -e, or ensure check() always returns 0, or call check in a way that suppresses set -e (e.g., check ... || true).
Proposed fix (option A — always return 0 from check)
check() {
TOTAL_CHECKS=$((TOTAL_CHECKS + 1))
if eval "$2"; then
print_success "$1"
PASSED_CHECKS=$((PASSED_CHECKS + 1))
- return 0
else
print_error "$1"
FAILED_CHECKS=$((FAILED_CHECKS + 1))
- return 1
fi
+ return 0
}Proposed fix (option B — remove set -e)
-set -e
+# Intentionally not using set -e; check() and warn() track failures via counters.Also applies to: 56-67
🤖 Prompt for AI Agents
In `@scripts/verify_ios_deployment.sh` at line 9, The script currently uses set -e
which causes the first failing check() to abort execution and prevents the
consolidated report using PASSED_CHECKS/FAILED_CHECKS/WARNING_CHECKS from being
printed; either remove the set -e line, or modify calls to check (the check
function) to be invoked as "check ... || true" so failures don't trigger exit,
or change check() to always return 0 while still updating FAILED_CHECKS; update
invocations that rely on set -e accordingly so the summary printing block still
runs.
1 participant
Fixes - Jira-#501 Add missing common modules that exist in the kmp template project
This PR is in reference to #3098. Due to some conflicts there, a new PR has been made. Thanks
Screenshots
build.mp4
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
Infrastructure & Automation
Improvements