chore: Sync root level scripts, files and metadata from kmp-project-template#2569
Conversation
…emplate - Added core-base module structure from KMP template - Update settings.gradle to include core-base module(disabled now, pending build-logic setup) - Added/Updated root level scripts and metadata
📝 WalkthroughWalkthroughThis PR introduces a comprehensive Kotlin Multiplatform project template with eight core modules: analytics, common utilities, database abstraction, design system, networking, platform managers, and UI utilities. Includes documentation, build configurations, scripts for keystore and directory management, and platform-specific implementations across Android, iOS, Desktop, Web, and Native targets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ViewModel as BaseViewModel
participant StateFlow
participant EventChannel
participant UI as Composable
User->>ViewModel: trySendAction(action)
ViewModel->>ViewModel: internalActionChannel.send(action)
ViewModel->>ViewModel: handleAction(action)
ViewModel->>StateFlow: mutableStateFlow.emit(newState)
StateFlow->>UI: state update notification
UI->>UI: recompose with new state
ViewModel->>EventChannel: sendEvent(event)
EventChannel->>UI: event emission
UI->>UI: EventsEffect handler
sequenceDiagram
participant Client
participant KtorHttpClient
participant Engine as Engine(OkHttp/Darwin/JS)
participant Server
participant ResultConverter as ResultSuspendConverterFactory
Client->>KtorHttpClient: httpClient.request()
KtorHttpClient->>Engine: send request
Engine->>Server: HTTP call
Server-->>Engine: response
Engine-->>ResultConverter: HttpResponse
alt Success (2xx)
ResultConverter->>ResultConverter: deserialize body
ResultConverter-->>Client: NetworkResult.Success(data)
else Client Error (4xx)
ResultConverter->>ResultConverter: map status code
ResultConverter-->>Client: NetworkResult.Error(NetworkError)
else Server Error (5xx)
ResultConverter-->>Client: NetworkResult.Error(SERVER)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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
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: 17
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (26)
core-base/platform/src/androidMain/kotlin/template/core/base/platform/utils/AndroidBuildUtils.kt-10-10 (1)
10-10: Replace placeholder package name before module integration.The package name uses
templateas the root namespace, which is a placeholder from the KMP project template. This pattern occurs across the entire core-base module (95+ files). Before enabling the core-base module, refactor all package declarations fromtemplate.core.base.*tocom.mifos.core.base.*to match the project's package structure.core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/core/ComponentStateHolder.kt-19-48 (1)
19-48: Remove or revise the thread-safety claim in the KDoc.Line 22 states "This class provides a thread-safe way to hold and update state values," but
mutableStateOfis not thread-safe. It is designed exclusively for the single-threaded Compose runtime and must only be accessed from the main/UI thread. This misleading claim could encourage unsafe concurrent access patterns and should be removed or clarified.core-base/ui/src/commonMain/kotlin/template/core/base/ui/SharedElementExt.kt-12-12 (1)
12-12: Package name retains "template" prefix from source.The package name
template.core.base.uiappears to be a placeholder from the kmp-project-template. Before enabling the core-base module in settings.gradle, ensure this is renamed to align with the Mifos X Field Officer project namespace (e.g.,org.mifos.core.base.uior similar).core-base/network/build.gradle.kts-15-17 (1)
15-17: Update the template placeholder namespace to match the project.The namespace
"template.core.base.network"appears to be a placeholder from the KMP template. It should be updated to align with the project's actual package structure (e.g.,"com.mifos.core.base.network"or"org.mifos.core.base.network").🔎 Proposed fix
android { - namespace = "template.core.base.network" + namespace = "com.mifos.core.base.network" }Note: Adjust the namespace prefix (
com.mifos) to match your project's established package naming convention.core-base/ui/src/commonMain/kotlin/template/core/base/ui/ImageLoaderExt.kt-53-65 (1)
53-65: Make DebugLogger conditional on build type.The
DebugLogger()is unconditionally enabled, which means verbose logging will run in production builds. This can impact performance and potentially leak information in production logs.🔎 Suggested approach
Consider making the logger conditional based on build type:
fun getDefaultImageLoader(context: PlatformContext): ImageLoader = ImageLoader .Builder(context) - .logger(DebugLogger()) + .apply { + // Only enable debug logging in debug builds + if (BuildConfig.DEBUG) { + logger(DebugLogger()) + } + } .networkCachePolicy(CachePolicy.ENABLED) .memoryCachePolicy(CachePolicy.ENABLED)Alternatively, pass the logger as a parameter to allow configuration at the call site, or use a platform-specific expectation to determine whether debug logging should be enabled.
Committable suggestion skipped: line range outside the PR's diff.
core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-80-80 (1)
80-80: Replace println with structured logging.Using
printlnfor error and diagnostic messages is not production-ready. These statements lack log levels, structure, and proper output channels. Since Kermit is already in use elsewhere in the project (seeKtorHttpClient.kt), integrate it here for consistent, configurable logging.🔎 Recommended approach
Add Kermit logger instance to the class and replace println statements:
+import co.touchlab.kermit.Logger + class ResultSuspendConverterFactory : Converter.Factory { + private val logger = Logger.withTag("ResultSuspendConverterFactory") + override fun suspendResponseConverter( typeData: TypeData, ktorfit: Ktorfit, ): Converter.SuspendResponseConverter<HttpResponse, *>? { if (typeData.typeInfo.type == NetworkResult::class) { val successType = typeData.typeArgs.first().typeInfo return object : Converter.SuspendResponseConverter<HttpResponse, NetworkResult<Any, NetworkError>> { override suspend fun convert(result: KtorfitResult): NetworkResult<Any, NetworkError> { return when (result) { is KtorfitResult.Failure -> { - println("Failure: " + result.throwable.message) + logger.e(result.throwable) { "Request failed" } NetworkResult.Error(NetworkError.UNKNOWN) } is KtorfitResult.Success -> { val status = result.response.status.value when (status) { 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}") + logger.e(e) { "Serialization failed" } NetworkResult.Error(NetworkError.SERIALIZATION) } } 400 -> NetworkResult.Error(NetworkError.BAD_REQUEST) 401 -> NetworkResult.Error(NetworkError.UNAUTHORIZED) 404 -> NetworkResult.Error(NetworkError.NOT_FOUND) 408 -> NetworkResult.Error(NetworkError.REQUEST_TIMEOUT) 429 -> NetworkResult.Error(NetworkError.TOO_MANY_REQUESTS) in 500..599 -> NetworkResult.Error(NetworkError.SERVER) else -> { - println("Status code $status") + logger.w { "Unhandled status code: $status" } NetworkResult.Error(NetworkError.UNKNOWN) } } } } } } } return null } }Also applies to: 95-95, 107-107
core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt-144-148 (1)
144-148: Fix overly broad host matching in the logging filter.The
request.url.host.contains(host)check at line 146 performs substring matching, which can match unintended hosts. For example, ifloggableHostsincludes"api", it will match both"api.example.com"(intended) and"maliciousapi.com"(unintended). This could lead to logging sensitive requests to unauthorized hosts or failing to log legitimate ones.🔎 Proposed fix using equality or suffix matching
Option 1: Exact equality (if hosts are fully qualified)
filter { request -> loggableHosts.any { host -> - request.url.host.contains(host) + request.url.host == host } }Option 2: Domain suffix matching (if hosts are domain patterns)
filter { request -> loggableHosts.any { host -> - request.url.host.contains(host) + request.url.host == host || request.url.host.endsWith(".$host") } }Choose based on whether
loggableHostscontains exact hostnames or domain patterns.core-base/database/src/desktopMain/kotlin/template/core/base/database/AppDatabaseFactory.kt-104-114 (1)
104-114: Handle potential nullAPPDATAand unchecked directory creation.Two potential issues:
System.getenv("APPDATA")can returnnullon Windows in unusual configurations, causing an NPE when passed toFile().mkdirs()can returnfalseif directory creation fails, but this is silently ignored.🔎 Proposed fix
val os = System.getProperty("os.name").lowercase() val userHome = System.getProperty("user.home") val appDataDir = when { - os.contains("win") -> File(System.getenv("APPDATA"), "MifosDatabase") + os.contains("win") -> { + val appData = System.getenv("APPDATA") ?: userHome + File(appData, "MifosDatabase") + } os.contains("mac") -> File(userHome, "Library/Application Support/MifosDatabase") else -> File(userHome, ".local/share/MifosDatabase") } if (!appDataDir.exists()) { - appDataDir.mkdirs() + require(appDataDir.mkdirs()) { "Failed to create database directory: $appDataDir" } }core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt-55-79 (1)
55-79: Remove the non-standardentityparameter from theInsertannotation.The
Insertannotation includes anentity: KClass<*>parameter that does not exist in Room's standard@InsertAPI. Room's@Insertannotation has only one parameter:onConflict. Keeping this extra parameter creates API incompatibility and will confuse developers familiar with Room. Remove theentityparameter to align with Room's established API contract.core-base/ui/src/desktopMain/java/template/core/base/ui/ShareUtils.desktop.kt-21-26 (1)
21-26:shareTextimplementation incorrectly saves text as an image file.The function uses
FileKit.saveImageToGallerywith text bytes and a.txtfilename, which is semantically incorrect.saveImageToGalleryis designed for image data, not text files. This will likely fail or produce unexpected results.Consider using a file picker/saver API appropriate for text files instead.
core-base/ui/src/desktopMain/java/template/core/base/ui/ShareUtils.desktop.kt-28-38 (1)
28-38:shareImagemay produce corrupt image files.
image.asSkiaBitmap().readPixels()returns raw pixel data (RGBA bytes), not an encoded PNG. Saving this directly with a.pngextension will create a file that image viewers cannot open.The pixel data needs to be encoded to PNG format before saving. Consider using Skia's encoding APIs to properly encode the image.
core-base/platform/src/nonAndroidMain/kotlin/template/core/base/platform/intent/IntentManagerImpl.kt-32-38 (1)
32-38: Inconsistent placeholder implementations will cause runtime crashes.Lines 33 and 37 use
TODO("Not yet implemented")which throwsNotImplementedErrorat runtime, while other methods use commented// TODOand silently do nothing. This inconsistency means callingshareFile(fileUri, mimeType, extraText)orshareImage(title, image)on non-Android platforms will crash the app unexpectedly.For consistency with other placeholder methods, either comment these TODOs or document that these operations are intentionally unsupported on non-Android platforms.
🔎 Proposed fix for consistent no-op behavior
override fun shareFile(fileUri: String, mimeType: MimeType, extraText: String) { - TODO("Not yet implemented") + // TODO("Not yet implemented") } override suspend fun shareImage(title: String, image: ImageBitmap) { - TODO("Not yet implemented") + // TODO("Not yet implemented") }Committable suggestion skipped: line range outside the PR's diff.
keystore-manager.sh-638-646 (1)
638-646:sed -iwithout backup suffix is not portable to macOS.Line 638 uses
sed -iwithout a backup suffix, which works on GNU sed (Linux) but fails on BSD sed (macOS). In contrast,update_fastlane_config(line 572) correctly usessed -i.bak.🔎 Proposed fix for macOS compatibility
# Use sed to update the signing configuration - sed -i \ + sed -i.bak \ -e "s|storeFile = file(System.getenv(\"KEYSTORE_PATH\") ?: \".*\")|storeFile = file(System.getenv(\"KEYSTORE_PATH\") ?: \"../keystores/$keystore_name\")|" \ -e "s|storePassword = System.getenv(\"KEYSTORE_PASSWORD\") ?: \".*\"|storePassword = System.getenv(\"KEYSTORE_PASSWORD\") ?: \"$keystore_password\"|" \ -e "s|keyAlias = System.getenv(\"KEYSTORE_ALIAS\") ?: \".*\"|keyAlias = System.getenv(\"KEYSTORE_ALIAS\") ?: \"$key_alias\"|" \ -e "s|keyPassword = System.getenv(\"KEYSTORE_ALIAS_PASSWORD\") ?: \".*\"|keyPassword = System.getenv(\"KEYSTORE_ALIAS_PASSWORD\") ?: \"$key_password\"|" \ "$gradle_file" + + # Remove the backup file + rm -f "$gradle_file.bak"Committable suggestion skipped: line range outside the PR's diff.
core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/PerformanceTracker.kt-231-233 (1)
231-233: Variable shadows top-levelcurrentTimeproperty.Line 232 declares a local
val currentTimethat shadows the top-level property. While this happens to work because the shadowing creates a fresh evaluation in the local scope, it's confusing and fragile. After fixing the top-levelcurrentTimeto be a getter, this shadowing should be removed:fun markAppForeground() { - val currentTime = currentTime - lastForegroundTime = currentTime + lastForegroundTime = currentTime val bgTime = backgroundTimecore-base/analytics/src/androidProd/kotlin/template.core.base.analytics/di/AnalyticsModule.kt-10-12 (1)
10-12: Fix inconsistent source set directory structure in androidProd.The
@file:Suppress("InvalidPackageDeclaration")is necessary becauseandroidProduses a non-standard directory structure with dots in folder names (template.core.base.analytics/di/) instead of the standard slash-based structure (template/core/base/analytics/di/) used by other source sets likeandroidDemoandcommonMain. To eliminate this suppression and align with Kotlin conventions, rename the androidProd directory fromtemplate.core.base.analytics/totemplate/core/base/analytics/.core-base/platform/src/androidMain/kotlin/template/core/base/platform/garbage/GarbageCollectionManager.android.kt-12-14 (1)
12-14: Remove explicit garbage collection calls or document specific use case.Explicit calls to
Runtime.getRuntime().gc()are strongly discouraged in Android applications per official Android Developers guidance. They can trigger expensive full garbage collections and often worsen performance. The current implementation compounds this concern by making 10 repeated GC attempts in a loop, which is even more problematic than a single call.If this is intentional for a specific, measured use case (e.g., memory testing, controlled server-side scenarios), document the reasoning and measurement results. Otherwise, reconsider this abstraction entirely and focus on reducing allocation churn instead.
core-base/common/src/nonAndroidMain/kotlin/template/core/base/common/manager/DispatchManagerImpl.kt-29-30 (1)
29-30:appScopecreates a newCoroutineScopeon every access.Using
get() = CoroutineScope(...)creates a fresh scope each time the property is accessed. This defeats the purpose of a shared application scope — coroutines launched in different accesses won't share the sameSupervisorJoband won't be cancelled together.Proposed fix using lazy initialization
- override val appScope: CoroutineScope - get() = CoroutineScope(SupervisorJob() + Dispatchers.Default) + override val appScope: CoroutineScope by lazy { + CoroutineScope(SupervisorJob() + Dispatchers.Default) + }Alternatively, use direct initialization:
override val appScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Default)core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/layout/KptFlowRow.kt-26-27 (1)
26-27:horizontalArrangementandverticalAlignmentparameters are unused.These parameters are declared but never applied in the layout logic. Items are always placed sequentially from x=0 without arrangement spacing, and cross-axis positioning ignores vertical alignment.
🔎 Suggested approach
Apply
horizontalArrangementwhen calculating x positions within each row, and useverticalAlignmentto align items within rows of varying heights. Alternatively, if these features are not yet needed, consider removing the parameters to avoid misleading the API consumers.core-base/common/src/androidMain/kotlin/template/core/base/common/manager/DispatchManagerImpl.kt-29-30 (1)
29-30:appScopecreates a new scope on every access.Using a getter with
SupervisorJob()creates a freshCoroutineScopeeach timeappScopeis accessed. This defeats the purpose of having a shared application-wide scope for long-running operations.🔎 Proposed fix - use lazy initialization
- override val appScope: CoroutineScope - get() = CoroutineScope(SupervisorJob() + Dispatchers.Default) + override val appScope: CoroutineScope by lazy { + CoroutineScope(SupervisorJob() + Dispatchers.Default) + }core-base/platform/src/androidMain/kotlin/template/core/base/platform/intent/IntentManagerImpl.kt-224-242 (1)
224-242: FileOutputStream not closed properly on error paths.The
FileOutputStreamis created on line 231 butflush()orclose()could throw, leaving the stream open. Use Kotlin'suseextension for automatic resource management.🔎 Proposed fix using use{} block
private suspend fun saveImage(image: Bitmap, context: Context): Uri? { return withContext(Dispatchers.IO) { try { val imagesFolder = File(context.cacheDir, "images") imagesFolder.mkdirs() val file = File(imagesFolder, "shared_image.png") - val stream = FileOutputStream(file) - image.compress(Bitmap.CompressFormat.PNG, 100, stream) - stream.flush() - stream.close() + FileOutputStream(file).use { stream -> + image.compress(Bitmap.CompressFormat.PNG, 100, stream) + stream.flush() + } FileProvider.getUriForFile(context, "${context.packageName}.provider", file) } catch (e: IOException) { Log.d("saving bitmap", "saving bitmap error ${e.message}") null } } }core-base/platform/src/androidMain/kotlin/template/core/base/platform/intent/IntentManagerImpl.kt-95-102 (1)
95-102: Missing FLAG_ACTIVITY_NEW_TASK when starting activity from non-Activity context.When
contextis an Application or Service context (not Activity), callingstartActivitywithoutFLAG_ACTIVITY_NEW_TASKwill crash on some Android versions. Consider adding the flag for safety.🔎 Proposed fix
} else { val newUri = if (androidUri.scheme == null) { androidUri.buildUpon().scheme("https").build() } else { androidUri.normalizeScheme() } - startActivity(Intent(Intent.ACTION_VIEW, newUri)) + startActivity(Intent(Intent.ACTION_VIEW, newUri).apply { + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + }) }core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/core/KptTopAppBarConfiguration.kt-116-116 (1)
116-116: Typo in property name:onNavigationIonClickshould beonNavigationIconClick.The property name
onNavigationIonClickappears to be missing the "c" in "Icon". This is in the public API and should be fixed before release to avoid breaking changes later.🔎 Proposed fix
- val onNavigationIonClick: (() -> Unit)? = null, + val onNavigationIconClick: (() -> Unit)? = null,Also update line 246 in the builder:
- onNavigationIonClick = onNavigationClick, + onNavigationIconClick = onNavigationClick,And update the KDoc on line 97:
- * @param onNavigationIonClick Optional click handler for the navigation icon + * @param onNavigationIconClick Optional click handler for the navigation iconcore-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.kt-88-106 (1)
88-106: FileOutputStream resource leak - same issue as IntentManagerImpl.The
FileOutputStreamshould useuse{}block to ensure proper cleanup. This is duplicated code fromIntentManagerImpl.saveImage.🔎 Proposed fix
private suspend fun saveImage(image: Bitmap, context: Context): Uri? { return withContext(Dispatchers.IO) { try { val imagesFolder = File(context.cacheDir, "images") imagesFolder.mkdirs() val file = File(imagesFolder, "shared_image.png") - val stream = FileOutputStream(file) - image.compress(Bitmap.CompressFormat.PNG, 100, stream) - stream.flush() - stream.close() + FileOutputStream(file).use { stream -> + image.compress(Bitmap.CompressFormat.PNG, 100, stream) + stream.flush() + } FileProvider.getUriForFile(context, "${context.packageName}.provider", file) } catch (e: IOException) { Log.d("saving bitmap", "saving bitmap error ${e.message}") null } } }core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/layout/AdaptiveNavigableListDetailScaffold.kt-258-261 (1)
258-261: Non-null assertionitem.id!!may throw NPE.The
PaneScaffoldItem<T>interface allowsTto be nullable, but line 260 usesitem.id!!which will crash ifidis null. Consider constraining the type or handling nullability.🔎 Proposed fix - use safe key or require non-null id
itemsIndexed( items = items, - key = { _, item -> item.id!! }, + key = { index, item -> item.id ?: index }, ) { index, item ->Alternatively, constrain the interface:
interface PaneScaffoldItem<T : Any> { val id: T }core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/component/KptAnimationSpecs.kt-105-123 (1)
105-123:emphasizedEasingandstandardEasingdo not match Material Design 3 specifications.Both use
CubicBezierEasing(0.2f, 0.0f, 0.0f, 1.0f), but according to MD3 specs:
standardEasingshould beCubicBezierEasing(0.4f, 0.0f, 0.2f, 1.0f)emphasizedEasingshould match one of the MD3 emphasized approximations: either decelerate(0.05f, 0.7f, 0.1f, 1.0f)or accelerate(0.3f, 0.0f, 0.8f, 0.15f)The file claims to follow Material Design 3 guidelines but these values deviate from the spec. Correct these to match the intended easing curves, or update the documentation if using custom values.
core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/component/KptTopAppBar.kt-382-394 (1)
382-394:KptMediumTopAppBarandKptLargeTopAppBarignore theonNavigationIconClickparameter.Both functions accept
onNavigationIconClickas a parameter but never use it - they always call the simplerKptTopAppBar(title, modifier, variant)overload which doesn't support navigation.🔎 Proposed fix
@Composable fun KptMediumTopAppBar( title: String, onNavigationIconClick: (() -> Unit)? = null, modifier: Modifier = Modifier, -) = KptTopAppBar(title, modifier, TopAppBarVariant.Medium) +) = onNavigationIconClick?.let { + KptTopAppBar( + title = title, + onNavigationIconClick = it, + modifier = modifier, + variant = TopAppBarVariant.Medium, + ) +} ?: KptTopAppBar(title, modifier, TopAppBarVariant.Medium) @Composable fun KptLargeTopAppBar( title: String, onNavigationIconClick: (() -> Unit)? = null, modifier: Modifier = Modifier, -) = KptTopAppBar(title, modifier, TopAppBarVariant.Large) +) = onNavigationIconClick?.let { + KptTopAppBar( + title = title, + onNavigationIconClick = it, + modifier = modifier, + variant = TopAppBarVariant.Large, + ) +} ?: KptTopAppBar(title, modifier, TopAppBarVariant.Large)Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (48)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/Transition.kt-63-67 (1)
63-67: Documentation refers to "slide transitions" but describes push transitions.The KDoc for
DEFAULT_PUSH_TRANSITION_TIME_MSis copy-pasted fromDEFAULT_SLIDE_TRANSITION_TIME_MSand incorrectly says "slide transitions."/** - * The default transition time (in milliseconds) for all slide transitions in the + * The default transition time (in milliseconds) for all push transitions in the * [TransitionProviders]. */ const val DEFAULT_PUSH_TRANSITION_TIME_MS: Int = 350core-base/ui/src/commonMain/kotlin/template/core/base/ui/NavGraphBuilderExtensions.kt-8-8 (1)
8-8: Minor typo in license header."See See" should be just "See".
- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEcore-base/ui/src/commonMain/kotlin/template/core/base/ui/Transition.kt-8-8 (1)
8-8: Minor typo in license header.Same typo as in
NavGraphBuilderExtensions.kt: "See See" should be "See".- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEcore-base/ui/src/commonMain/kotlin/template/core/base/ui/SharedElementExt.kt-10-10 (1)
10-10: RemoveExperimentalSharedTransitionApiopt-in annotations if using Compose Animation 1.10+.The
SharedTransitionApiwas stabilized inandroidx.compose.animation:1.10.0(released December 3, 2025). If your project is on this version or later, the@file:OptIn(ExperimentalSharedTransitionApi::class)annotation (lines 10, 15) is no longer required and should be removed.core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/core/KptComponent.kt-8-8 (1)
8-8: Typo: duplicate word "See".The license comment has "See See" which should be just "See".
🔎 Proposed fix
- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEsync-dirs.sh-312-316 (1)
312-316: Unreachable cleanup afterhandle_error.
handle_errorcallsexit 1, socleanup_temp_dirson line 314 will never execute. Consider using a trap for cleanup or calling cleanup before the error handler.Proposed fix using trap
Add a trap at the beginning of the script (after line 22):
# Cleanup trap for unexpected exits trap 'find . -depth -type d -name "temp_*" -exec rm -rf {} + 2>/dev/null; rm -rf temp_files temp_root' EXITThen simplify the error block:
git checkout "$temp_branch" -- "$dir" || { - handle_error "Failed to sync $dir" - cleanup_temp_dirs + handle_error "Failed to sync $dir" }Committable suggestion skipped: line range outside the PR's diff.
sync-dirs.sh-149-204 (1)
149-204: Unused parametercheck_dirand potential prefix matching issue.
The
check_dirparameter (line 151) is declared but never used in the function body, which suggests either dead code or a missing implementation.Line 183: The pattern
[[ "$full_path" == "$dir"* ]]could match unintentionally if directory names share prefixes (e.g., a hypothetical exclusion keycmpwould match paths starting withcmp-android). Consider using"$dir/"*to ensure proper directory boundary matching.Proposed fix
is_excluded() { - local check_dir=$1 - local full_path=$2 - local check_type=$3 # 'file' or 'dir' + local full_path=$1 + local check_type=$2 # 'file' or 'dir' # Remove ./ from the beginning of the path if it exists full_path="${full_path#./}"And update the call site at line 327:
- if is_excluded "$(dirname "$file")" "$file" "file"; then + if is_excluded "$file" "file"; thenFor the prefix matching, consider:
- if [[ "$full_path" == "$dir"* ]]; then + if [[ "$full_path" == "$dir/"* || "$full_path" == "$dir" ]]; thenCommittable suggestion skipped: line range outside the PR's diff.
core-base/common/src/commonMain/kotlin/template/core/base/common/DataState.kt-8-8 (1)
8-8: Typo: Duplicate "See" in comment.The line reads "See See" which appears to be a copy-paste error.
🔎 Proposed fix
- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEcore-base/common/src/commonMain/kotlin/template/core/base/common/DataStateExtensions.kt-8-8 (1)
8-8: Typo: Duplicate "See" in comment.Same typo as in DataState.kt.
🔎 Proposed fix
- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEcore-base/common/src/commonMain/kotlin/template/core/base/common/ImageExtension.kt-91-102 (1)
91-102: Edge case: Empty MIME type returns empty string instead of null.For data URIs like
"data:;base64,..."(valid per RFC 2397, defaulting to text/plain),extractMimeTypeFromDataUrireturns an empty string rather than null. This behavior may be unexpected given the nullable return type typically signals invalidity.Consider either:
- Returning null for empty MIME types to maintain consistency with the nullable return type semantics, or
- Documenting that empty strings are valid and represent the default MIME type
🔎 Proposed fix to return null for empty MIME types
fun String.extractMimeTypeFromDataUri(): String? { val dataUriPrefix = "data:" val base64Prefix = ";base64," return takeIf { it.startsWith(dataUriPrefix) && it.contains(base64Prefix) } ?.substringAfter(dataUriPrefix) ?.substringBefore(base64Prefix) + ?.takeIf { it.isNotEmpty() } }core-base/database/src/androidMain/kotlin/template/core/base/database/AppDatabaseFactory.kt-8-8 (1)
8-8: Fix typo in license header."See See" should be "See" (duplicate word).
core-base/database/src/nativeMain/kotlin/template/core/base/database/AppDatabaseFactory.kt-8-8 (1)
8-8: Fix typo in license header."See See" should be "See" (duplicate word).
core-base/database/src/nonJsCommonMain/kotlin/template/core/base/database/Room.nonJsCommon.kt-8-8 (1)
8-8: Fix typo in license header."See See" should be "See" (duplicate word).
core-base/database/src/desktopMain/kotlin/template/core/base/database/AppDatabaseFactory.kt-8-8 (1)
8-8: Fix typo in license header."See See" should be "See" (duplicate word).
core-base/database/src/commonMain/kotlin/template/core/base/database/TypeConverter.kt-8-8 (1)
8-8: Fix typo in license header."See See" should be "See" (duplicate word).
🔎 Proposed fix
- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEcore-base/database/README.md-191-196 (1)
191-196: Minor grammatical improvement.Per static analysis, "iOS/macOS specific" should be hyphenated as a compound adjective.
🔎 Proposed fix
- Leverages Kotlin/Native C interop for platform APIs -- Requires iOS/macOS specific Room dependencies +- Requires iOS/macOS-specific Room dependenciescore-base/database/src/nonJsCommonMain/kotlin/template/core/base/database/TypeConverter.nonJsCommon.kt-8-8 (1)
8-8: Fix typo in license header."See See" should be "See" (duplicate word).
🔎 Proposed fix
- * See See https://github.com/openMF/kmp-project-template/blob/main/LICENSE + * See https://github.com/openMF/kmp-project-template/blob/main/LICENSEcore-base/database/build.gradle.kts-1-20 (1)
1-20: Remove duplicate copyright header and unused import.The file contains a duplicate copyright header (lines 1-9 and 12-20). Additionally, line 10 imports
org.jetbrains.compose.composewhich is unused in this build file.🔎 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/kmp-project-template/blob/main/LICENSE - */ -import org.jetbrains.compose.compose - /* * Copyright 2025 Mifos Initiative *core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt-218-236 (1)
218-236: Clarify the documentation forOnConflictStrategy.NONE.The
OnConflictStrategy.NONEconstant is a standard Room constant (not custom), so no removal is needed. However, the documentation comment is inaccurate. According to Room's documentation, NONE (the default) has runtime behavior identical to ABORT—it rolls back the transaction when conflicts occur.Update the comment from:
/** No conflict resolution strategy specified (may cause exceptions) */ const val NONE = 0to something like:
/** Default strategy (no ON CONFLICT clause). Runtime behavior same as ABORT (rolls back transaction) */ const val NONE = 0This prevents confusion about the actual behavior.
core-base/platform/build.gradle.kts-1-20 (1)
1-20: Remove duplicate copyright header.The copyright header appears twice (lines 1-10 and lines 12-20). Remove one of the duplicate blocks.
🔎 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/kmp-project-template/blob/main/LICENSE */ 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 { alias(libs.plugins.kmp.library.convention)core-base/ui/build.gradle.kts-1-20 (1)
1-20: Remove duplicate license header.The license header appears twice (lines 1-9 and lines 12-20), with an import statement sandwiched between them. Remove the duplicate header block at lines 12-20.
🔎 Proposed fix
+import org.jetbrains.kotlin.gradle.ExperimentalKotlinGradlePluginApi + /* * 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 */ -import org.jetbrains.kotlin.gradle.ExperimentalKotlinGradlePluginApi - -/* - * 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 { alias(libs.plugins.kmp.library.convention) alias(libs.plugins.jetbrainsCompose)core-base/platform/README.md-73-76 (1)
73-76: Truncated documentation text.Lines 75-76 appear to be incomplete:
- Line 75 ends with
"android.content.Context")- Line 76 ends abruptly with
"- \AppContext.activity`: Provides access to the current activity (e.g., `android`"Please complete this documentation.
core-base/designsystem/README.md-30-30 (1)
30-30: Fix documentation formatting issues.Address the following markdown formatting issues for better readability:
- Lines 30 & 331: Add language specifiers to fenced code blocks (e.g.,
textor appropriate language)- Line 341: Replace emphasis with a proper heading (e.g.,
## Built with ❤️ for the Kotlin Multiplatform community)🔎 Proposed fixes
For line 30:
-``` +```text designsystem/For line 331:
-``` +```text Copyright 2025 Mifos InitiativeFor line 341:
-**Built with ❤️ for the Kotlin Multiplatform community** +## Built with ❤️ for the Kotlin Multiplatform communityAlso applies to: 331-331, 341-341
core-base/common/build.gradle.kts-47-47 (1)
47-47: Add newline at end of file.The file is missing a newline at the end, which is a common convention to prevent issues with some tools and diff utilities.
🔎 Proposed fix
jsMain.dependencies { api(libs.jb.kotlin.stdlib.js) api(libs.jb.kotlin.dom) } } } +core-base/platform/src/nonAndroidMain/kotlin/template/core/base/platform/review/AppReviewManagerImpl.kt-19-21 (1)
19-21: Track or remove the TODO comment.The TODO comment suggests implementing a custom review flow, but this is the non-Android stub where platform-specific review flows may not be applicable. Consider whether this TODO is necessary for non-Android targets, or if it should be removed.
Would you like me to open an issue to track this task, or shall we remove the TODO if custom review flow isn't needed for non-Android platforms?
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BackgroundEvent.kt-1-17 (1)
1-17: Copyright year inconsistency.The copyright year is 2024 while other files in this PR use 2025. Consider updating for consistency.
🔎 Proposed fix
/* - * Copyright 2024 Mifos Initiative + * Copyright 2025 Mifos Initiative * * This Source Code Form is subject to the terms of the Mozilla PublicThe marker interface design is appropriate for its purpose.
core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/layout/KptSplitPane.kt-34-36 (1)
34-36: Unused parameters:minLeftWidth,minRightWidth,resizable.These parameters are declared but never referenced in the implementation. Either:
- Remove them if resize functionality is not planned, or
- Implement the resize logic (drag handle on divider) and min-width constraints.
This appears to be incomplete functionality synced from the template.
Would you like me to help implement the resize functionality, or should these parameters be removed for now?
core-base/platform/src/androidMain/kotlin/template/core/base/platform/update/AppUpdateManagerImpl.kt-96-98 (1)
96-98: IncorrectLog.dparameter order.
Log.d(String tag, String msg, Throwable tr)expects tag first, then message. The current call has them reversed.Proposed fix
}.addOnFailureListener { - Log.d("Unable to update app!", "UpdateManager", it) + Log.e("UpdateManager", "Unable to update app!", it) }Also changed to
Log.esince this is an error condition.core-base/ui/src/commonMain/kotlin/template/core/base/ui/EventsEffect.kt-29-42 (1)
29-42: Misleading parameter name and missing key dependency.Two issues:
- Parameter
lifecycleOwnerhas typeLifecycle, notLifecycleOwner— consider renaming tolifecyclefor clarity.LaunchedEffect(key1 = Unit)won't restart ifviewModelchanges. If the same composable can receive different viewModels, the effect will continue observing the stale one.Proposed fix
@Composable fun <E> EventsEffect( viewModel: BaseViewModel<*, E, *>, - lifecycleOwner: Lifecycle = LocalLifecycleOwner.current.lifecycle, + lifecycle: Lifecycle = LocalLifecycleOwner.current.lifecycle, handler: suspend (E) -> Unit, ) { - LaunchedEffect(key1 = Unit) { + LaunchedEffect(key1 = viewModel) { viewModel.eventFlow .filter { it is BackgroundEvent || - lifecycleOwner.currentState.isAtLeast(Lifecycle.State.RESUMED) + lifecycle.currentState.isAtLeast(Lifecycle.State.RESUMED) } .onEach { handler.invoke(it) } .launchIn(this) } }core-base/platform/src/androidMain/kotlin/template/core/base/platform/update/AppUpdateManagerImpl.kt-116-136 (1)
116-136: Missing error handling incheckForResumeUpdateState.Unlike
checkForAppUpdate, this method has no.addOnFailureListener. If the update info fetch fails, errors will be silently swallowed.Proposed fix
override fun checkForResumeUpdateState() { manager .appUpdateInfo .addOnSuccessListener { appUpdateInfo -> if (appUpdateInfo.updateAvailability() == UpdateAvailability.DEVELOPER_TRIGGERED_UPDATE_IN_PROGRESS ) { // If an in-app update is already running, resume the update. manager.startUpdateFlowForResult( /* p0 = */ appUpdateInfo, /* p1 = */ activity, /* p2 = */ updateOptions, /* p3 = */ UPDATE_MANAGER_REQUEST_CODE, ) } + }.addOnFailureListener { + Log.e("UpdateManager", "Failed to check resume update state", it) } }core-base/ui/src/commonMain/kotlin/template/core/base/ui/EventsEffect.kt-53-66 (1)
53-66: Same issues as the first overload: misleading name and static key.Apply the same fixes for consistency: rename
lifecycleOwner→lifecycle, and useeventFlowas theLaunchedEffectkey so the effect restarts when the flow reference changes.Proposed fix
@Composable fun <E> EventsEffect( eventFlow: Flow<E>, - lifecycleOwner: Lifecycle = LocalLifecycleOwner.current.lifecycle, + lifecycle: Lifecycle = LocalLifecycleOwner.current.lifecycle, handler: suspend (E) -> Unit, ) { - LaunchedEffect(key1 = Unit) { + LaunchedEffect(key1 = eventFlow) { eventFlow .filter { it is BackgroundEvent || - lifecycleOwner.currentState.isAtLeast(Lifecycle.State.RESUMED) + lifecycle.currentState.isAtLeast(Lifecycle.State.RESUMED) } .onEach { handler.invoke(it) } .launchIn(this) } }core-base/platform/src/androidMain/kotlin/template/core/base/platform/update/AppUpdateManagerImpl.kt-85-94 (1)
85-94: Deprecated API:startUpdateFlowForResultwith request code.This overload is deprecated in Play Core library. The modern approach uses
ActivityResultLauncher<IntentSenderRequest>withAppUpdateOptions, or the Task-basedstartUpdateFlow()API. Update to use the AppUpdateOptions-based overloads.core-base/platform/src/commonMain/kotlin/template/core/base/platform/LocalManagerProviders.kt-55-60 (1)
55-60: Documentation typo: "circumstance manager" should be "app update manager".The KDoc comment incorrectly refers to "circumstance manager" instead of "app update manager."
🔎 Proposed fix
/** - * Provides access to the circumstance manager throughout the app. + * Provides access to the app update manager throughout the app. */ val LocalAppUpdateManager: ProvidableCompositionLocal<AppUpdateManager> = compositionLocalOf {core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/AnalyticsEvent.kt-268-277 (1)
268-277: Documentation claimsIllegalArgumentExceptionbut implementation silently truncates.The KDoc for
Param(line 238) andwithParam(line 70) states@throws IllegalArgumentException if validation constraints are violated, but theinvokefactory silently truncates values and uses a fallback key instead of throwing. This is a documentation-behavior mismatch.Either update the documentation to reflect the defensive behavior, or throw exceptions as documented. The current defensive approach is actually preferable for analytics (avoids crashes), so updating the docs is recommended:
🔎 Proposed doc fix for Param class
- * @throws IllegalArgumentException if validation constraints are violated + * Keys exceeding 40 characters are truncated; blank keys fall back to "unknown_param". + * Values exceeding 100 characters are truncated.Committable suggestion skipped: line range outside the PR's diff.
core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/UiHelpers.kt-40-52 (1)
40-52: Potential duplicate screen view logging whenadditionalParamsis provided.When
additionalParamsis not empty, bothlogScreenView(line 41) andlogEventwithTypes.SCREEN_VIEW(lines 44-51) are called, potentially resulting in two screen view events being logged for the same navigation.🔎 Proposed fix - consolidate into single event
LaunchedEffect(screenName) { - analytics.logScreenView(screenName, sourceScreen) - // Log additional params if provided if (additionalParams.isNotEmpty()) { analytics.logEvent( Types.SCREEN_VIEW, mapOf( ParamKeys.SCREEN_NAME to screenName, *additionalParams.toList().toTypedArray(), ).plus(sourceScreen?.let { mapOf(ParamKeys.SOURCE_SCREEN to it) } ?: emptyMap()), ) + } else { + analytics.logScreenView(screenName, sourceScreen) } }core-base/platform/src/androidMain/kotlin/template/core/base/platform/review/AppReviewManagerImpl.kt-54-56 (1)
54-56: IncorrectLog.eparameter order.
Log.e(tag, message)expects the tag as the first parameter, but the code passes the message first. This will result in misleading log output.🔎 Proposed fix
} else { - Log.e("Failed to launch review flow.", task.exception?.message.toString()) + Log.e("AppReviewManager", "Failed to launch review flow: ${task.exception?.message}") }core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt-69-82 (1)
69-82: Incomplete URL encoding may cause malformed URLs.The
encode()function only handles spaces and newlines, but URL encoding requires escaping many more characters (&,=,?,#,%,+, etc.). Consider using a proper URL encoding utility to prevent malformed URLs or unexpected behavior.core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptMaterialTheme.kt-72-78 (1)
72-78: KDoc parameter documentation mismatch.The documentation references
@param darkThemetwice with different meanings:
- Line 73: Boolean flag for dark theme
- Line 74: Describes it as "KptThemeProvider for dark theme"
The actual parameter is named
darkThemeProvider(line 83), but the doc says@param darkTheme.🔎 Proposed fix
/** * KptMaterialTheme with dark theme support. * Provides automatic light/dark theme switching with Material3 integration. * * @param darkTheme Whether to use dark theme. Defaults to system preference. * @param lightTheme KptThemeProvider for light theme - * @param darkTheme KptThemeProvider for dark theme + * @param darkThemeProvider KptThemeProvider for dark theme * @param content The composable content that will have access to both KptTheme and MaterialTheme * * @sample KptMaterialThemeWithDarkModeExample */core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt-84-96 (1)
84-96: SMS URL uses incorrect query separator.The SMS URI should use
?to start the query string, not&. The formatsms:$number&body=may not work correctly on iOS.🔎 Proposed fix
actual fun sendViaSMS(number: String, message: String) { fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") val encodedMessage = encode(message) val smsUrl = if (number.isNotEmpty()) { - "sms:$number&body=$encodedMessage" + "sms:$number?body=$encodedMessage" } else { - "sms:&body=$encodedMessage" + "sms:?body=$encodedMessage" } val url = NSURL.URLWithString(smsUrl) if (url != null && UIApplication.sharedApplication.canOpenURL(url)) { UIApplication.sharedApplication.openURL(url) } }core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt-22-30 (1)
22-30:keyWindowis deprecated in iOS 13+; use the scenes API instead.
UIApplication.sharedApplication().keyWindowis deprecated in iOS 13 and later. For iOS 13+, access the key window viaUIApplication.shared.connectedScenes:val currentViewController = UIApplication.shared.connectedScenes .compactMap { ($0 as? UIWindowScene)?.keyWindow } .last?.rootViewControllerOr for broader compatibility (iOS 13+):
val currentViewController = UIApplication.shared.connectedScenes .compactMap { $0 as? UIWindowScene } .flatMap { $0.windows } .first { $0.isKeyWindow }?.rootViewControllercore-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/layout/AdaptiveNavigableSupportingPaneScaffold.kt-59-103 (1)
59-103: Consider using the stable BackHandler API from androidx.activity.compose instead of androidx.compose.ui.backhandler.BackHandler.The androidx.compose.ui.backhandler.BackHandler is experimental and requires the ExperimentalComposeUiApi opt-in. For production Android code, the stable androidx.activity.compose.BackHandler (from androidx.activity:activity-compose) is the recommended approach. If multiplatform support is a requirement, the experimental API is acceptable, but be aware of the stability implications across platform-specific implementations.
core-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.kt-108-116 (1)
108-116: Redundantletblock does not use its parameter.Line 110 uses
url.let { url.toUri() }but ignores theitparameter, making theletunnecessary.🔎 Proposed fix
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) }core-base/platform/src/androidMain/kotlin/template/core/base/platform/intent/IntentManagerImpl.kt-56-63 (1)
56-63: Unsafe cast may throw ClassCastException.The
intent as Intentcast on line 59 will throwClassCastExceptionif caller passes a non-Intent object. Since the interface acceptsAny, consider adding a type check or documenting the expected type contract.🔎 Proposed fix with type validation
override fun startActivity(intent: Any) { + if (intent !is Intent) { + Log.w("IntentManagerImpl", "startActivity called with non-Intent: ${intent::class}") + return + } try { Log.d("IntentManagerImpl", "Starting activity: $intent") - context.startActivity(intent as Intent) + context.startActivity(intent) } catch (_: ActivityNotFoundException) { // no-op } }core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/ValidationUtils.kt-295-308 (1)
295-308: Validation error logging could exceed parameter limits.The
analytics_validation_errorevent includeserrorsparameter with joined error messages. If the original event had many errors, this could exceedMAX_PARAM_VALUE_LENGTH(100 chars). The error event itself should be validated/sanitized.🔎 Proposed fix
if (logValidationErrors) { delegate.logEvent( AnalyticsEvent( "analytics_validation_error", listOf( - Param(key = "original_event_type", value = event.type), + Param( + key = "original_event_type", + value = event.type.take(AnalyticsValidator.MAX_PARAM_VALUE_LENGTH) + ), Param( key = "errors", - value = validationResult.errorMessages.joinToString("; "), + value = validationResult.errorMessages.joinToString("; ") + .take(AnalyticsValidator.MAX_PARAM_VALUE_LENGTH), ), ), ), ) }core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/KptColorSchemeImpl.kt-246-299 (1)
246-299:KptColorSchemeBuilderis missing several color properties fromKptColorSchemeImpl.The builder exposes only 22 color properties, but
KptColorSchemeImplhas 37 (missing:scrim,inverseSurface,inverseOnSurface,inversePrimary,surfaceDim,surfaceBright,surfaceContainerLowest,surfaceContainerLow,surfaceContainer,surfaceContainerHigh,surfaceContainerHighest,surfaceTint).Users cannot customize these colors via the builder DSL.
🔎 Suggested additions to KptColorSchemeBuilder
Add the missing properties to the builder class:
var scrim: Color = Color(0xFF000000) var inverseSurface: Color = Color(0xFF313033) var inverseOnSurface: Color = Color(0xFFF4EFF4) var inversePrimary: Color = Color(0xFFD0BCFF) var surfaceDim: Color = Color(0xFFDAD6DC) var surfaceBright: Color = Color(0xFFFFFBFE) var surfaceContainerLowest: Color = Color(0xFFFFFFFF) var surfaceContainerLow: Color = Color(0xFFF3EFF4) var surfaceContainer: Color = Color(0xFFE7E0EC) var surfaceContainerHigh: Color = Color(0xFFDAD6DC) var surfaceContainerHighest: Color = Color(0xFFCFC8D0) var surfaceTint: Color = Color(0xFF6750A4)And include them in the
build()function.core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/KptColorSchemeImpl.kt-301-336 (1)
301-336:KptTypographyBuilderdefaults differ fromKptTypographyImpldefaults.The builder's default
TextStylevalues only setfontWeightandfontSize, butKptTypographyImplincludeslineHeightandletterSpacing. This means typography built via the DSL won't match the default typography unless explicitly configured.Consider either copying the full defaults from
KptTypographyImplor delegating toKptTypographyImpl()for initial values.core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptThemeExtensions.kt-295-297 (1)
295-297: Inconsistent use ofthis.primaryinstead ofthis.surfaceTintforsurfaceTint.Line 297 maps
surfaceTinttothis.primary, butKptColorSchemehas a dedicatedsurfaceTintproperty. This could lead to incorrect colors if someone customizessurfaceTintseparately fromprimary.🔎 Proposed fix
- surfaceTint = this.primary, + surfaceTint = this.surfaceTint,core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/component/KptTopAppBar.kt-77-89 (1)
77-89: Fix typo:onNavigationIonClickshould beonNavigationIconClickThe property name in
KptTopAppBarConfigurationis misspelled (missing 'c' in "Icon"), causing API inconsistency. The public function overloads inKptTopAppBar.ktuse the correct spellingonNavigationIconClickas parameters but must map them to the misspelled property.Fix the property definition in
KptTopAppBarConfigurationand all 8 usages inKptTopAppBar.kt(lines 80, 81, 178, 200, 222, 244, 311, 346).
core-base/analytics/src/androidProd/kotlin/template.core.base.analytics/di/AnalyticsModule.kt
Show resolved
Hide resolved
core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/PerformanceTracker.kt
Show resolved
Hide resolved
...-base/common/src/androidMain/kotlin/template/core/base/common/manager/DispatchManagerImpl.kt
Show resolved
Hide resolved
core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt
Show resolved
Hide resolved
core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt
Show resolved
Hide resolved
core-base/platform/src/commonMain/kotlin/template/core/base/platform/di/PlatformModule.kt
Show resolved
Hide resolved
...rm/src/commonMain/kotlin/template/core/base/platform/garbage/GarbageCollectionManagerImpl.kt
Show resolved
Hide resolved
.../platform/src/nonAndroidMain/kotlin/template/core/base/platform/context/AppContext.native.kt
Show resolved
Hide resolved
core-base/ui/src/commonMain/kotlin/template/core/base/ui/ImageLoaderExt.kt
Show resolved
Hide resolved
core-base/ui/src/commonMain/kotlin/template/core/base/ui/LifecycleEventEffect.kt
Show resolved
Hide resolved
|
@amanna13 If you are looking to contribute to mobile projects at Mifos, please go through this document Welcome to the Mifos Mobile Apps Community to get started. |
|
@biplab1 Thanks for sharing this! I already gone thru the guide and have joined the Slack as well. This PR focuses only on syncing missing root level files/folders and keeping behavior unchanged. The initial PR is necessarily large, but changes are mostly additive and mirror the template structure. Hence I intentionally kept it as one single PR instead of splitting it. Please let me know if there’s anything specific you’d like me to do next for this PR. Thanks! |
|
@amanna13 Have you registered for the daily standup meeting? There is a link in the document itself. If you don't find it, let me know in the Slack mobile channel. |
Yess !! I'm already in the meet. |
HekmatullahAmin
left a comment
HekmatullahAmin
left a comment
There was a problem hiding this comment.
@amanna13 just add some files that needs to be excluded from detekt also https://github.com/openMF/mobile-wallet/pull/1895/files#diff-ea29de1180303564fca0c4eddb30f7da4b596754f2c139fc252ae290e3d201e2
|
@amanna13 Have you addressed the above comment yet? |
…/android-client into chore/sync-kmp-template
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @config/detekt/detekt.yml:
- Around line 454-464: The detekt excludes array is missing the iOS platform
pattern which causes false positives for files like FileKitUtil.ios.kt and
ClientListScreen.ios.kt; update the excludes list (the array under "excludes")
by adding the pattern "**/*.ios.*" alongside the existing entries (e.g.,
"**/*.android.*", "**/*.desktop.*") so iOS-specific Kotlin files are ignored by
MatchingDeclarationName.
In @secrets.env:
- Around line 10-18: The keystore password entries
(ORIGINAL_KEYSTORE_FILE_PASSWORD, ORIGINAL_KEYSTORE_ALIAS_PASSWORD,
UPLOAD_KEYSTORE_FILE_PASSWORD, UPLOAD_KEYSTORE_ALIAS_PASSWORD) use weak
hard-coded values; replace those concrete passwords with non-functional
placeholders like <YOUR_PASSWORD_HERE> (or an environment variable reference)
and add a brief comment documenting required complexity (length, character
classes, and not reusing file/alias passwords) so the file is a safe template
rather than containing guessable credentials.
- Around line 36-42: The file mixes real organization values (COMPANY_NAME and
ORGANIZATION set to "Mifos Initiative.") with a template keystore alias
("kmp-project-template"); decide whether this is a template or a real config and
make values consistent: if it's a template, replace COMPANY_NAME and
ORGANIZATION with clear placeholders like "<Your Organization Name>" and update
CITY/STATE/COUNTRY placeholders as needed and document that developers must
replace them before release; if it's an actual android-client config, replace
the keystore alias "kmp-project-template" with your real alias and ensure
COMPANY_NAME and ORGANIZATION contain the official organization name (no
trailing dot) throughout.
- Around line 1-118: secrets.env contains committed sensitive keystore material
and hardcoded passwords (e.g., ORIGINAL_KEYSTORE_FILE, UPLOAD_KEYSTORE_FILE,
ORIGINAL_KEYSTORE_FILE_PASSWORD, UPLOAD_KEYSTORE_ALIAS_PASSWORD); remove the
file from the repo immediately and purge it from git history using a
history-rewrite tool (git filter-repo or BFG) targeting secrets.env and the
embedded BASE64 blobs, then add secrets.env to .gitignore, create a safe
secrets.env.example with placeholder values for keys like
ORIGINAL_KEYSTORE_NAME/UPLOAD_KEYSTORE_NAME and FIREBASECREDS/PLAYSTORECREDS,
update the README with instructions for developers to generate/populate a local
secrets.env, and rotate any keystores or credentials that were signed or
exposed.
- Around line 48-55: The commit contains real base64 PKCS12 keystores committed
as environment variables (ORIGINAL_KEYSTORE_FILE and UPLOAD_KEYSTORE_FILE);
remove these entries immediately, purge them from git history (eg. git
filter-repo/BFG), add the keystore variable names/keystore files to .gitignore,
and replace the inline values with references to a secure secret store (e.g.,
environment variables loaded at runtime or a secrets manager) used by your
CI/CD; then regenerate new keystores locally, rotate/revoke the compromised
signing certs and update any CI/CD signing configuration (the code paths that
read ORIGINAL_KEYSTORE_FILE/UPLOAD_KEYSTORE_FILE and the related passwords) to
fetch from the secret manager instead of the repo.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoreconfig/detekt/detekt.ymlsecrets.env
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: amanna13
Repo: openMF/android-client PR: 2569
File: core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/layout/KptResponsiveLayout.kt:140-153
Timestamp: 2025-12-31T08:19:28.678Z
Learning: In the android-client project, when syncing template code from kmp-project-template, functional or behavioral fixes in the synced template code are intentionally deferred to follow-up issues to keep the sync PR scoped to template syncing only.
🔇 Additional comments (2)
config/detekt/detekt.yml (2)
435-435: LGTM!Excluding generated code from function naming checks is a sensible addition for a KMP project, preventing false positives from annotation processors and code generation tools.
480-483: LGTM!Excluding generated directories from package naming checks is consistent with the FunctionNaming exclusion and prevents false positives from code generation tools.
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
4 participants
Fixes - Jira-#597
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.
This PR syncs missing root files and folders from KMP project template into Mifos X Field Officer
Note
core-basemodule from the kmp-template are currently added, but commented out in settings.gradle, as they depend on KMP conventtion plugins and version-catalog entries that will be introduced in the follow-up epic tasks. ( Check: Jira-#596 )Note
The existing
detektconfiguration inandroid-clientwas kept as is. The templatedetekt.ymlis significantly more extensive and depends on build-logic and modules that will be introduced in later epic steps. ( Check: Jira-#596 )Summary by CodeRabbit
Release Notes
New Features
Documentation
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.