Fix build failures across all KMP targets (JVM, JS, WASM, iOS)#2608
Fix build failures across all KMP targets (JVM, JS, WASM, iOS)#2608Arinyadav1 wants to merge 11 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughMultiplatform refactor: enabled JS/wasm targets, removed iosX64, added a KMP base library convention plugin, refactored Room expect/actual annotations, removed a DB migration and downgraded VERSION, introduced many JS/WASM/desktop/native stub implementations and FileKit/storage adjustments, and bumped paging/build settings. Changes
Sequence Diagram(s)(omitted — changes are broad refactors and many small stubs; no single new multi-component runtime flow introduced) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
cmp-android/src/main/kotlin/cmp/android/app/ComponentActivityExtensions.kt
Outdated
Show resolved
Hide resolved
cmp-android/src/main/kotlin/cmp/android/app/ComponentActivityExtensions.kt
Outdated
Show resolved
Hide resolved
...re/savings/savingsAccountTransactionReceipt/SavingsAccountTransactionReceiptScreen.wasmJs.kt
Show resolved
Hide resolved
...eature/savings/savingsAccountTransactionReceipt/SavingsAccountTransactionReceiptScreen.js.kt
Show resolved
Hide resolved
feature/report/src/wasmJsMain/kotlin/com/mifos/feature/report/reportDetail/FileHelper.wasmJs.kt
Show resolved
Hide resolved
feature/report/src/jsMain/kotlin/com/mifos/feature/report/reportDetail/FileHelper.js.kt
Show resolved
Hide resolved
...-tracking/src/wasmJsMain/kotlin/com/mifos/feature/path/tracking/PathTrackingScreen.wasmJs.kt
Outdated
Show resolved
Hide resolved
...ure/path-tracking/src/jsMain/kotlin/com/mifos/feature/path/tracking/PathTrackingScreen.js.kt
Outdated
Show resolved
Hide resolved
...e/groups/src/wasmJsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.wasmJs.kt
Outdated
Show resolved
Hide resolved
feature/groups/src/jsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.js.kt
Outdated
Show resolved
Hide resolved
...ure/client/src/wasmJsMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.wasmJs.kt
Outdated
Show resolved
Hide resolved
feature/client/src/jsMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.js.kt
Outdated
Show resolved
Hide resolved
...nter/src/wasmJsMain/kotlin/com/mifos/feature/center/centerList/ui/CenterListScreen.wasmJs.kt
Outdated
Show resolved
Hide resolved
feature/center/src/jsMain/kotlin/com/mifos/feature/center/centerList/ui/CenterListScreen.js.kt
Outdated
Show resolved
Hide resolved
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/FileKitUtil.kt
Outdated
Show resolved
Hide resolved
core/database/src/commonMain/kotlin/com/mifos/room/typeconverters/CustomTypeConverters.kt
Outdated
Show resolved
Hide resolved
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/checkerinboxtask/CheckerTask.kt
Outdated
Show resolved
Hide resolved
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/responses/SaveResponse.kt
Outdated
Show resolved
Hide resolved
feature/center/src/jsMain/kotlin/com/mifos/feature/center/centerList/ui/CenterListScreen.js.kt
Outdated
Show resolved
Hide resolved
...nter/src/wasmJsMain/kotlin/com/mifos/feature/center/centerList/ui/CenterListScreen.wasmJs.kt
Outdated
Show resolved
Hide resolved
feature/groups/src/jsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.js.kt
Outdated
Show resolved
Hide resolved
...e/groups/src/wasmJsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.wasmJs.kt
Outdated
Show resolved
Hide resolved
|
@Arinyadav1 I hope you have figured out the FileKit part, since it will break several screens as those parts have been commented out. Please make sure that it is resolved in the next PR. Also, ensure that the image cropping issue is taken care of in another PR immediately after that. |
|
@biplab1 Yeah sure after merge this PR I will fix that issue also |
|
@biplab1 please review it again |
|
@Arinyadav1 Some of the previously raised issues are still unresolved. Kindly address them, or explicitly comment if a fix is not required. Please notify me once this is done. |
|
@biplab1 I have fixed all that issues you can check and anyone is left please mention me with issue, I have comment on all issues.. |
|
@biplab1 please review it again I have done the changes |
|
@Arinyadav1 Is every target building fine after the updates? |
|
@biplab1 Yeah Working fine in all targets |
# Conflicts: # cmp-android/dependencies/demoDebugRuntimeClasspath.txt # cmp-android/dependencies/demoReleaseRuntimeClasspath.txt # cmp-android/dependencies/prodDebugRuntimeClasspath.txt # cmp-android/dependencies/prodReleaseRuntimeClasspath.txt # core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt # core-base/database/src/nonJsCommonMain/kotlin/template/core/base/database/Room.nonJsCommon.kt # core/database/src/androidMain/kotlin/com/mifos/room/MifosDatabase.kt # core/database/src/androidMain/kotlin/com/mifos/room/di/DatabaseModule.android.kt # core/database/src/desktopMain/kotlin/com/mifos/room/di/DatabaseModule.desktop.kt # core/database/src/jsMain/kotlin/com/mifos/room/di/DatabaseModule.common.js.kt # core/database/src/jsMain/kotlin/com/mifos/room/utils/GetCurrentTimeInMillis.js.kt # core/database/src/nativeMain/kotlin/com/mifos/room/di/PlatformSpecificModule.kt # core/database/src/wasmJsMain/kotlin/com/mifos/room/di/DatabaseModule.common.wasmJs.kt # core/database/src/wasmJsMain/kotlin/com/mifos/room/utils/GetCurrentTimeInMillis.wasmJs.kt # core/ui/src/desktopMain/kotlin/com/mifos/core/ui/util/ShareUtils.desktop.kt # feature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/utils/OpenPdfInDefaultExternalApp.wasmJs.kt # gradle.properties # gradle/libs.versions.toml # gradle/wrapper/gradle-wrapper.properties
Arinyadav1
force-pushed
the
Fix-build-failures
branch
from
3825ddf to
2cf29f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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 (22)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDocuments/ClientDocumentsViewModel.kt (2)
200-228:⚠️ Potential issue | 🔴 CriticalLoading dialog is never dismissed on download success — UI is permanently stuck.
DataState.LoadingtriggersloadingDialogState()(line 201), but the entireDataState.Successblock is now a no-op comment.nullDialogState()is commented out, so the loading spinner is never cleared after a successful download. The user is left with a frozen, unresponsive loading state.Even though the FileKit integration is being deferred,
nullDialogState()must remain active in the success branch to keep the dialog lifecycle correct.🐛 Minimal fix — dismiss the dialog even while the rest is deferred
is DataState.Success -> { // TODO: writeFileToCache return type has changed because currently not support all // KMP target that's why currently comment out this it will fix in next PR + nullDialogState() // documentSelectAndUploadRepository.updateEntityDocument(platformFile = dataState.data) -// nullDialogState() // if (dataState.data.extension == "pdf") { // sendAction(ClientDocumentsActions.OpenExternalPdfViewer(dataState.data)) // } else { // sendEvent(ClientDocumentsEvents.OnViewDocument) // } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDocuments/ClientDocumentsViewModel.kt` around lines 200 - 228, The success branch currently does nothing and never dismisses the loading UI; inside the is DataState.Success branch in ClientDocumentsViewModel (the block handling DataState.Success), call nullDialogState() to dismiss the loading dialog (before or immediately after any further actions) and then continue with the existing sendEvent(ClientDocumentsEvents.OnViewDocument) if appropriate; leave the rest of the deferred file-handling lines commented out but ensure nullDialogState() is executed so the spinner is cleared on download success.
107-109:⚠️ Potential issue | 🟡 Minor
OpenExternalPdfViewerhandler andpreviewPdfInExternalAppare now unreachable dead code.The only site that dispatches
ClientDocumentsActions.OpenExternalPdfViewer(previously line 210 in the commented-out block) is disabled.previewPdfInExternalApp(lines 230–240) and theOpenExternalPdfViewerbranch inhandleActioncan never be reached. Similarly,ClientDocumentsEvents.OnViewDocument(line 311) can never be sent.These can remain as stubs if the follow-up PR will restore the functionality, but it's worth tracking explicitly so they aren't forgotten.
Do you want me to open a tracking issue for restoring full FileKit/document-viewing support in the follow-up PR?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDocuments/ClientDocumentsViewModel.kt` around lines 107 - 109, ClientDocumentsActions.OpenExternalPdfViewer, previewPdfInExternalApp, and the ClientDocumentsEvents.OnViewDocument emission are currently dead/unreachable because the only dispatcher was disabled; either remove these unreachable branches and helper function from ClientDocumentsViewModel or explicitly retain them as stubs with a clear TODO and tracking reference so they aren't lost. Update the handleAction switch (the OpenExternalPdfViewer branch) and the previewPdfInExternalApp method: if you keep them, add a TODO comment linking to a tracking issue (or open one now) and mark the methods/events as intentionally noop/stub; if you remove them, also remove any related imports/usages (e.g., references to ClientDocumentsEvents.OnViewDocument) to avoid unused code. Ensure you update/tests or comments to reflect which approach was taken.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)
352-368:⚠️ Potential issue | 🟠 MajorImage upload is silently broken and
selectedImagePathnow permanently returnsnull.Since both
selectedImagePath = file.pathassignments are commented out,selectedImagePath(Line 347) staysnullfor the entire composition lifetime. This has two cascading regressions:
No visual feedback –
ClientImageSection(selectedImagePath = selectedImagePath)(Line 470) always renders the placeholder, so users see no indication that their image was picked.Upload never triggered –
handleSubmitClickpassesselectedImagePath(alwaysnull) tosetFileForUpload.invoke(...)(Line 794). The callback checksfilePath?.let { createClientWithImage = true }(Lines 221–224), socreateClientWithImageis never set totrue. WhenSetClientIdis reached (Lines 232–238), the screen always falls tonavigateBack()instead of callinguploadImage(uiState.id). ThePlatformFilestored in the ViewModel viaonImageSelected(file)is therefore silently discarded on every target.The cleanest minimal fix is to decouple the upload trigger from the now-unavailable path. Since
onImageSelectedalready propagates thePlatformFileto the ViewModel, introduce a dedicated flag to drivecreateClientWithImage:🛠️ Proposed fix – decouple upload trigger from path
var selectedImagePath by rememberSaveable { mutableStateOf<String?>(null) } + var imageSelected by rememberSaveable { mutableStateOf(false) }val galleryLauncher = rememberFilePickerLauncher( type = FileKitType.Image, ) { file -> file?.let { -// TODO: path not support in kmp all targets -// selectedImagePath = file.path + // TODO: display selected image preview (path API unavailable cross-platform) + imageSelected = true onImageSelected(file) } } val cameraLauncher = rememberPlatformCameraLauncher { file -> file?.let { -// TODO: path not support in kmp all targets -// selectedImagePath = file.path + // TODO: display selected image preview (path API unavailable cross-platform) + imageSelected = true onImageSelected(file) } }Then replace the
setFileForUploadcall-site in the SubmitonClickwith:- setFileForUpload, + imageSelected,And update
handleSubmitClicksignature/body accordingly:- selectedImagePath: String?, - setFileForUpload: (filePath: String?) -> Unit, + imageSelected: Boolean, + setFileForUpload: (selected: Boolean) -> Unit,- setFileForUpload.invoke(selectedImagePath) + setFileForUpload.invoke(imageSelected)And the lambda in the stateful overload:
- setFileForUpload = { filePath -> - filePath?.let { - createClientWithImage = true - } - }, + setFileForUpload = { selected -> + if (selected) createClientWithImage = true + },For image preview, the ViewModel could expose the
PlatformFile(or aByteArray) as aStateFlowsoClientImageSectioncan render it without relying on a file path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 352 - 368, selectedImagePath is never set because both selectedImagePath = file.path lines in the rememberFilePickerLauncher and rememberPlatformCameraLauncher callbacks are commented out, so the UI always shows a placeholder and upload logic never triggers; instead, modify the flow so onImageSelected(file) also flips a dedicated flag (e.g., imageSelectedForUpload or hasSelectedImage) in the ViewModel/state when invoked from rememberFilePickerLauncher and rememberPlatformCameraLauncher, update setFileForUpload.invoke(...) call-site in the Submit onClick to pass that flag (or call a new setCreateClientWithImage(true) method) rather than relying on a file path, and adjust handleSubmitClick, createClientWithImage handling, and ClientImageSection to read the ViewModel/state-provided flag and/or PlatformFile (exposed as StateFlow or ByteArray) so previews and upload trigger correctly without depending on file.path; use the existing symbols rememberFilePickerLauncher, rememberPlatformCameraLauncher, selectedImagePath, onImageSelected(file), setFileForUpload.invoke, handleSubmitClick, createClientWithImage, and ClientImageSection to locate and implement these changes.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
163-176:⚠️ Potential issue | 🟠 Major"Upload New Photo" button leads to a silent UX dead-end — disable it or add a placeholder dialog.
When the user taps "Upload New Photo" (Line 166),
ClientProfileEditAction.OnUploadNewPhotoClicksetsdialogState = ShowUploadOptionsin the ViewModel. However, theShowUploadOptionsbranch inClientProfileEditDialogs(Line 229) is completely empty—all UI code is commented out. No dialog is shown, and there is no mechanism to dismiss the state. The user sees no feedback and cannot proceed, leaving them stuck.Since the picker migration to
crop-krop-uiis deferred, either disable the button or display a temporary placeholder:Option 1: Disable the button
MifosTextButton( text = { Text(stringResource(Res.string.upload_new_photo)) }, onClick = { onAction(ClientProfileEditAction.OnUploadNewPhotoClick) }, modifier = Modifier.fillMaxWidth(), + enabled = false, // TODO(KMP): Re-enable once migrated to crop-krop-ui leadingIcon = { Icon( painter = painterResource(Res.drawable.arrow_up), contentDescription = null, modifier = Modifier.size(DesignToken.sizes.iconAverage), ) }, )Option 2: Show a temporary dialog
ClientProfileEditState.DialogState.ShowUploadOptions -> { // TODO(KMP): Migrate from cmp-image-pick-n-crop to crop-krop-ui in follow-up PR. + MifosBasicDialog( + visibilityState = BasicDialogState.Shown( + title = "Feature Unavailable", + message = "Photo upload will be available in an upcoming update.", + ), + onDismissRequest = { onAction(ClientProfileEditAction.DismissModalBottomSheet) }, + onConfirm = { onAction(ClientProfileEditAction.DismissModalBottomSheet) }, + confirmText = stringResource(Res.string.ok), + ) }Also consider removing the 25 lines of commented-out code (lines 232–256) to reduce noise; the TODO comment already documents the migration intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt` around lines 163 - 176, The "Upload New Photo" button triggers ClientProfileEditAction.OnUploadNewPhotoClick which sets dialogState = ShowUploadOptions but ClientProfileEditDialogs has an empty ShowUploadOptions branch, leaving users in a dead-end; fix by either disabling the MifosTextButton (set enabled=false or omit onClick) in ClientProfileEditProfileScreen until the picker migration is complete, or implement a simple placeholder dialog in ClientProfileEditDialogs for the ShowUploadOptions case that shows a dismissible message and clears dialogState on dismissal; also remove the large block of commented-out picker migration code in ClientProfileEditDialogs to reduce noise and rely on the existing TODO to document future work.core/database/src/jsMain/kotlin/com/mifos/room/utils/GetCurrentTimeInMillis.js.kt (1)
12-14:⚠️ Potential issue | 🟠 Major
TODO()stubs will throwNotImplementedErrorat runtime on the JS and WebAssembly targets.The PR aims to fix build failures across all KMP targets. Both
jsMainandwasmJsMainimplementations currently compile without error but throwNotImplementedErrorwhenevergetCurrentTimeInMillis()is called at runtime. Any database or time-dependent feature on these JS-based targets will crash.
kotlin.js.Dateis available in Kotlin/JS and provides millisecond-precision timestamps with zero additional dependencies:🛠️ Proposed implementation for JS and WebAssembly targets
+import kotlin.js.Date + actual fun getCurrentTimeInMillis(): Long { - TODO("Not yet implemented") + return Date().getTime().toLong() }Apply this fix to both:
core/database/src/jsMain/kotlin/com/mifos/room/utils/GetCurrentTimeInMillis.js.ktcore/database/src/wasmJsMain/kotlin/com/mifos/room/utils/GetCurrentTimeInMillis.wasmJs.kt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/database/src/jsMain/kotlin/com/mifos/room/utils/GetCurrentTimeInMillis.js.kt` around lines 12 - 14, Replace the TODO stub in the actual function getCurrentTimeInMillis() so it returns the current epoch millis using kotlin.js.Date instead of throwing NotImplementedError; update both implementations (the one in GetCurrentTimeInMillis.js.kt and the one in GetCurrentTimeInMillis.wasmJs.kt) to construct kotlin.js.Date() and return its getTime().toLong() (ensuring the return type Long) so JS and WASM targets no longer crash at runtime.core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/ShareUtils.js.kt (1)
56-57:⚠️ Potential issue | 🟡 Minor
openUrlis a no-op on JS but functional on WASM — likely a regression.The WASM counterpart implements
openUrlaswindow.open(url)(ShareUtils.wasmJs.kt line 57), andkotlinx.browser.windowis already imported here (line 13). This JS target should have the same behavior.Suggested fix
actual fun openUrl(url: String) { + window.open(url) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/jsMain/kotlin/com/mifos/core/ui/util/ShareUtils.js.kt` around lines 56 - 57, The openUrl function in ShareUtils.js.kt is currently a no-op; implement it to call the browser window opening API to match the WASM implementation: inside actual fun openUrl(url: String) call the imported kotlinx.browser.window.open(url) so the JS target opens the URL just like ShareUtils.wasmJs.kt does. Ensure the call is made in the function body of openUrl and keep the existing import of kotlinx.browser.window.core/common/src/commonMain/kotlin/com/mifos/core/common/utils/FileKitUtil.kt (1)
146-149:⚠️ Potential issue | 🟠 MajorComplete
takePhotoIfSupportedimplementations for JS and WASM targets with proper error handling.Two platform source sets are incomplete:
core/common/src/jsMain/kotlin/com/mifos/core/common/utils/FileKitUtil.js.kt(line 15): TODOcore/common/src/wasmJsMain/kotlin/com/mifos/core/common/utils/FileKitUtil.wasmJs.kt(line 15): TODOFollow the pattern established in
desktopMain(which correctly emitsDataState.Error(IllegalStateException("Platform not supported"))). This ensures callers receive a meaningful error state on unsupported platforms rather than a thrownNotImplementedErrorat runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/common/src/commonMain/kotlin/com/mifos/core/common/utils/FileKitUtil.kt` around lines 146 - 149, The JS and WASM platform implementations of expect fun takePhotoIfSupported() are left as TODOs and should match the desktopMain behavior by returning a Flow that emits a single DataState.Error with an IllegalStateException("Platform not supported") (instead of throwing NotImplementedError); update core/common/src/jsMain/kotlin/com/mifos/core/common/utils/FileKitUtil.js.kt and core/common/src/wasmJsMain/kotlin/com/mifos/core/common/utils/FileKitUtil.wasmJs.kt to implement takePhotoIfSupported() to emit DataState.Error(IllegalStateException("Platform not supported")) for PlatformFile? using the same Flow construction pattern used in desktopMain so callers receive a meaningful error state.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentifiersAddUpdateViewModel.kt (1)
53-65:⚠️ Potential issue | 🟡 Minor
Feature.valueOf()lacks error handling — add validation or explicit fallback.Line 56 calls
Feature.valueOf(route.feature)without error handling. While the current call site (ClientIdentifiersListScreen) always passes a validFeature.namevalue, any misconfigured navigation path or future deep link passing an invalid string will crash the screen ininitwithIllegalArgumentException.The second concern in the original review about
state.featurebeing read aftermutableStateFlow.update{}is not an issue:BaseViewModel.stateis a synchronous getter backed directly bymutableStateFlow.value, so the read is safe.Consider explicitly handling the enum parsing:
Suggested approach
init { + val parsedFeature = try { + Feature.valueOf(route.feature) + } catch (e: IllegalArgumentException) { + Feature.ADD_IDENTIFIER // or throw with more context + } mutableStateFlow.update { it.copy( clientId = route.clientId, - feature = Feature.valueOf(route.feature), + feature = parsedFeature, ) } - if (state.feature == Feature.ADD_IDENTIFIER) { + if (parsedFeature == Feature.ADD_IDENTIFIER) { getIdentifiersOptionsAndObserveNetwork() } else { viewModelScope.launch { getDocumentId() } } }Alternatively, validate at the navigation layer to ensure only valid Feature names are passed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentifiersAddUpdateViewModel.kt` around lines 53 - 65, The code calls Feature.valueOf(route.feature) without validation which can throw IllegalArgumentException for invalid strings; update ClientIdentifiersAddUpdateViewModel to safely parse the enum (e.g., use Feature.values().firstOrNull { it.name == route.feature } or runCatching) and choose a clear fallback (log/error state and bail out or default to a safe Feature), then call mutableStateFlow.update with the validated feature and proceed to either getIdentifiersOptionsAndObserveNetwork() or launch getDocumentId() based on that validated value; ensure any fallback path records the error (or navigates away) rather than letting the exception crash init.core/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanAccountEntity.kt (1)
73-77:⚠️ Potential issue | 🟠 MajorFix
collate = UNDEFINEDon line 73 — should useCollationSequence.UNSPECIFIED.Line 73 incorrectly uses
UNDEFINED(fromColumnInfoTypeAffinity) for thecollateparameter, while line 76 correctly usesUNSPECIFIED(fromCollationSequence). Thecollateparameter specifies a collation sequence (BINARY, NOCASE, RTRIM, etc.), not a type affinity. Both constants happen to have the same numeric value (1), so the code compiles and runs, but it's semantically incorrect and inconsistent. Thestatusfield should match theloanTypefield's usage:Proposed fix
- `@ColumnInfo`(index = true, name = INHERIT_FIELD_NAME, typeAffinity = UNDEFINED, collate = UNDEFINED, defaultValue = VALUE_UNSPECIFIED) + `@ColumnInfo`(index = true, name = INHERIT_FIELD_NAME, typeAffinity = UNDEFINED, collate = UNSPECIFIED, defaultValue = VALUE_UNSPECIFIED) val status: LoanStatusEntity? = null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanAccountEntity.kt` around lines 73 - 77, In LoanAccountEntity change the collate argument for the status ColumnInfo to use CollationSequence.UNSPECIFIED (not UNDEFINED); update the ColumnInfo on the status property (val status: LoanStatusEntity? = null) so its collate parameter matches the loanType field by importing/using CollationSequence.UNSPECIFIED for the collate value.core/database/src/androidMain/kotlin/com/mifos/room/MifosDatabase.kt (1)
156-158:⚠️ Potential issue | 🔴 Critical
ClientTypeConvertersremoval leavesLoanAccountEntityfields without required type converters.
ClientTypeConvertersprovided converters forLoanAccountEntity(used inClientAccounts.loanAccounts,GroupAccounts.loanAccounts, andCenterAccounts.loanAccounts/memberLoanAccounts). Removing it without adding equivalent converters toCustomTypeConverterswill cause runtime failures when attempting to serialize or deserialize these entity lists. Either restoreClientTypeConvertersto the@TypeConvertersannotation or addfromLoanAccountEntityandtoLoanAccountEntityconverter methods toCustomTypeConverters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/database/src/androidMain/kotlin/com/mifos/room/MifosDatabase.kt` around lines 156 - 158, The `@TypeConverters` annotation on MifosDatabase removed ClientTypeConverters which provided serialization for LoanAccountEntity used by ClientAccounts.loanAccounts, GroupAccounts.loanAccounts and CenterAccounts.memberLoanAccounts; restore functionality by either re-adding ClientTypeConverters to the `@TypeConverters` list or implement equivalent converter methods named fromLoanAccountEntity and toLoanAccountEntity inside CustomTypeConverters that handle converting LoanAccountEntity lists to/from a persistable format so those entity fields can be serialized/deserialized at runtime.core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt (1)
305-310:⚠️ Potential issue | 🟠 Major
Upsertannotation uses@Suppressinstead of@OptionalExpectation, inconsistent with 15 other annotations.
Upsertis the only annotation in this file that still uses the legacy@Suppress("NO_ACTUAL_FOR_EXPECT")pattern. All other annotations lacking actuals on JS/WASM (Dao,Query,Insert,Delete,Update,Entity,Transaction,ColumnInfo,Embedded,Relation,TypeConverter,Database,Ignore,DatabaseView) have been migrated to@OptIn(ExperimentalMultiplatform::class)@OptionalExpectation``. With@OptionalExpectation, the compiler properly handles missing actuals by dropping the annotation on unsupported platforms; with `@Suppress`, the warning is merely silenced without semantic handling.Proposed fix
-@Suppress("NO_ACTUAL_FOR_EXPECT") +@OptIn(ExperimentalMultiplatform::class) +@OptionalExpectation `@Target`(AnnotationTarget.FUNCTION) `@Retention`(AnnotationRetention.BINARY) expect annotation class Upsert( val entity: KClass<*>, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt` around lines 305 - 310, Replace the legacy suppression on the Upsert expect annotation with the modern optional-expect mechanism: in the Upsert declaration (expect annotation class Upsert), remove `@Suppress`("NO_ACTUAL_FOR_EXPECT") and add `@OptIn`(ExperimentalMultiplatform::class) and `@OptionalExpectation` so the compiler will drop the annotation on unsupported platforms rather than merely silencing warnings.core/common/build.gradle.kts (1)
46-50:⚠️ Potential issue | 🟡 MinorRemove unused
filekit.compose(v0.8.8) and redundantfilekit.dialogs.Lines 46–50 declare five FileKit dependencies, but two are problematic:
Line 48 (
libs.filekit.composev0.8.8): This old pre-v0.10 artifact is not imported anywhere in the codebase. All FileKit usage comes from v0.12.0 modules (filekit-core,filekit-dialogs,filekit-dialogs-compose). Remove this unused dependency.Line 50 (
libs.filekit.dialogs): Redundant. Thefilekit-dialogs-composedependency on line 49 already transitively includesfilekit-dialogs. Explicitly declaring it is unnecessary.Recommended action: Remove lines 48 and 50. Keep
filekit.core,filekit.coil, andfilekit.dialog.compose(v0.12.0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/common/build.gradle.kts` around lines 46 - 50, Remove the unused and redundant FileKit dependencies: delete the dependency entry referencing libs.filekit.compose (the old v0.8.8 artifact) and remove the explicit libs.filekit.dialogs declaration since filekit.dialogs is transitively provided by filekit.dialog.compose; keep the existing implementations of libs.filekit.core, libs.filekit.coil, and libs.filekit.dialog.compose so only those three remain in the implementation block.cmp-android/build.gradle.kts (1)
99-104:⚠️ Potential issue | 🟡 MinorDuplicate
dependencyGuardblocks — consolidate into one.
dependencyGuard { ... }is declared twice in the same file. In Gradle, the extension is configured additively across blocks, but having two separate declarations is confusing and risks divergence (e.g., theprodReleaseRuntimeClasspathentry appears in both blocks with different options).♻️ Proposed consolidation
-dependencyGuard { - configuration("demoDebugRuntimeClasspath") - configuration("demoReleaseRuntimeClasspath") - configuration("prodDebugRuntimeClasspath") - configuration("prodReleaseRuntimeClasspath") -} - -// ... rest of dependencies block ... - -dependencyGuard { - configuration("prodReleaseRuntimeClasspath") { - modules = true - tree = true - } -} +dependencyGuard { + configuration("demoDebugRuntimeClasspath") + configuration("demoReleaseRuntimeClasspath") + configuration("prodDebugRuntimeClasspath") + configuration("prodReleaseRuntimeClasspath") { + modules = true + tree = true + } +}Also applies to: 152-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-android/build.gradle.kts` around lines 99 - 104, The file declares duplicate dependencyGuard blocks which should be consolidated into a single configuration to avoid confusion and divergent entries; locate the multiple dependencyGuard { ... } blocks (they reference configurations like "demoDebugRuntimeClasspath", "demoReleaseRuntimeClasspath", "prodDebugRuntimeClasspath", and "prodReleaseRuntimeClasspath") and merge their configuration(...) lines into one dependencyGuard block so each runtimeClasspath entry appears only once and in a single place.cmp-web/src/wasmJsMain/kotlin/Main.kt (2)
57-57:⚠️ Potential issue | 🟡 MinorReplace
document.body!!with a null-safe guard.
document.bodycan technically benull(e.g., before<body>is parsed or in headless environments), causing an unhandledNullPointerExceptionthat crashes the WASM entry point.🛡️ Proposed fix
- ComposeViewport(document.body!!) { + val body = document.body ?: return@onWasmReady + ComposeViewport(body) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-web/src/wasmJsMain/kotlin/Main.kt` at line 57, The call to ComposeViewport(document.body!!) can throw an NPE because document.body may be null; update the WASM entry to null-check the body before passing it to ComposeViewport (e.g., guard with if (document.body == null) return or obtain a safe element via document.getElementsByTagName("body").firstOrNull() and return/log if null) so that ComposeViewport is only invoked with a non-null DOM element; adjust Main.kt accordingly around the ComposeViewport invocation.
58-89:⚠️ Potential issue | 🟠 Major
localeVersionis never incremented — locale changes have no visible effect on WASM.
key(localeVersion)is intended to force a full recomposition ofSharedAppwhen the locale changes, butlocaleVersionis never mutated insidehandleAppLocale. It stays0forever, sokey()never changes and Compose never restarts the subtree. Additionally,window.location.reload()is commented out with no alternative applied. The net effect: the user selects a language,localStorageis updated, but the UI is never refreshed — the locale change is silently swallowed.🐛 Proposed fix — increment `localeVersion` inside the lambda
handleAppLocale = { languageTag -> if (languageTag != null) { localStorage.setItem("app_language", languageTag) document.documentElement?.setAttribute("lang", languageTag) } else { localStorage.removeItem("app_language") val browserLang = window.navigator.language document.documentElement?.setAttribute("lang", browserLang) } + localeVersion++ // triggers key() recomposition },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-web/src/wasmJsMain/kotlin/Main.kt` around lines 58 - 89, The locale change handler never updates the remembered localeVersion, so key(localeVersion) never changes and SharedApp is not recomposed; inside the handleAppLocale lambda update the remembered state (localeVersion) after storing/removing localStorage and setting document.documentElement (e.g., increment localeVersion) so key(localeVersion) will rerun the composable; if you want a full page reload instead, either uncomment and use window.location.reload() or perform the increment+reload decision there, but do not leave localeVersion unchanged.core-base/database/build.gradle.kts (2)
1-22:⚠️ Potential issue | 🟡 MinorDuplicate copyright block — lines 1–9 and 12–20 are identical.
The file carries two consecutive copyright headers. One should be removed.
🔧 Remove the duplicate header
-/* - * 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/android-client/blob/master/LICENSE.md - */ import org.jetbrains.compose.compose /* * Copyright 2025 Mifos Initiative ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/build.gradle.kts` around lines 1 - 22, Remove the duplicated copyright header at the top of the file so only one copy remains: locate the two identical license comment blocks preceding the import statement and the plugins { alias(libs.plugins.kmp.core.base.library.convention) } declaration and delete the second block (the one immediately above the plugins block), leaving a single copyright header followed by import org.jetbrains.compose.compose and the plugins block.
10-10:⚠️ Potential issue | 🟡 MinorUnused import
org.jetbrains.compose.compose.No
compose.*symbol is referenced in this file (thekotlin.sourceSetsblocks only uselibs.androidx.room.runtime). This import is a leftover that should be removed.🔧 Remove the unused import
-import org.jetbrains.compose.compose - /* * Copyright 2025 Mifos Initiative🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/build.gradle.kts` at line 10, Remove the unused import org.jetbrains.compose.compose at the top of the build.gradle.kts file—it's not referenced anywhere (no compose.* symbols are used; the kotlin.sourceSets only reference libs.androidx.room.runtime), so delete that single import line to clean up the file.cmp-web/src/jsMain/kotlin/Application.kt (2)
36-61:⚠️ Potential issue | 🟠 Major
localeVersionis never incremented —key(localeVersion)never forces recomposition.
localeVersionis declared as mutable state and used as the argument tokey()to trigger a full recomposition ofSharedAppon locale change. However,handleAppLocalenever reassignslocaleVersion, so the key stays at0permanently. Thekey(localeVersion)block is effectively dead code: after a locale change, Compose UI strings will not update (only the DOMlangattribute is patched directly). The commented-outwindow.location.reload()at line 57 was the original working approach; the replacement recomposition mechanism is incomplete.🐛 Proposed fix — increment `localeVersion` at the end of the lambda
handleAppLocale = { languageTag -> if (languageTag != null) { localStorage.setItem("app_language", languageTag) document.documentElement?.setAttribute("lang", languageTag) } else { localStorage.removeItem("app_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() + localeVersion++ },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-web/src/jsMain/kotlin/Application.kt` around lines 36 - 61, The key-based recomposition never occurs because localeVersion (declared with var localeVersion by remember { mutableStateOf(0) }) is never changed; update the handleAppLocale lambda (the one passed into SharedApp) to increment localeVersion after applying localStorage/document changes so that key(localeVersion) will change and force a full recomposition of SharedApp (e.g., localeVersion += 1 or localeVersion++ at the end of handleAppLocale).
16-22:⚠️ Potential issue | 🟡 MinorStale comment — this is the JS entry point, not WebAssembly.
The docblock says "The entry point of the WebAssembly Compose application", but this file lives in
jsMain. The wasmJs target would have its own separate entry point. Update the comment to read "The entry point of the Kotlin/JS Compose application."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-web/src/jsMain/kotlin/Application.kt` around lines 16 - 22, The file-level docblock incorrectly calls this the "WebAssembly Compose application"; update the comment to say "The entry point of the Kotlin/JS Compose application." and remove or replace any other WebAssembly-specific phrasing so it correctly describes this JS entry point; you can find the comment near the top of Application.kt alongside references to onWasmReady, the Compose viewport creation, and the SharedApp composable—edit that comment text only.gradle/libs.versions.toml (2)
39-39:⚠️ Potential issue | 🟡 MinorRemove the orphaned version entry
cmpImagePickNCropat line 39.The library definition at line 200 is already commented out, but the corresponding version entry (
cmpImagePickNCrop = "1.1.2") remains in the [versions] section and serves no purpose. Since no activebuild.gradle.ktsfiles reference this library, cleanup is needed to remove dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/libs.versions.toml` at line 39, Remove the dead version entry named cmpImagePickNCrop from the [versions] section in gradle/libs.versions.toml; it's orphaned because the corresponding library definition is commented out and no build.gradle.kts references it, so delete the line cmpImagePickNCrop = "1.1.2" to clean up unused versions.
139-140:⚠️ Potential issue | 🟡 MinorRemove unused
filekit-compose:0.8.8dependency declarations.
filekit-compose:0.8.8is declared in 7 build.gradle files but is not imported or used anywhere in the codebase. All compose file-picking functionality has been migrated to the newfilekit-dialogs-compose:0.12.0API. The old artifact is dead code and can be safely removed.🛠️ Cleanup — remove all filekit-compose declarations
Remove from
gradle/libs.versions.toml:-fileKitCompose = "0.8.8"Remove from
gradle/libs.versions.toml(versions.bundles or [libraries]):-filekit-compose = { group = "io.github.vinceglb", name = "filekit-compose", version.ref = "fileKitCompose" }Remove from all
build.gradle.ktsfiles:-implementation(libs.filekit.compose)Affected modules:
feature/document,feature/client,core/ui,core/common,core-base/ui,cmp-android.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/libs.versions.toml` around lines 139 - 140, Remove the unused fileKitCompose declaration and all references to the old artifact: delete the fileKitCompose = "0.8.8" entry from gradle/libs.versions.toml and remove any usages or bundle entries named filekit-compose in the versions.bundles or [libraries] sections; update affected build.gradle.kts files in modules feature/document, feature/client, core/ui, core/common, core-base/ui, and cmp-android to stop importing or using the old artifact and, where needed, replace with the new filekit-dialogs-compose:0.12.0 alias (or remove the dependency entirely if no replacement is required). Ensure no remaining references to filekit-compose exist in the repo (dependencies, bundles, or import aliases) before committing.core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/PerformanceTracker.kt (1)
270-284:⚠️ Potential issue | 🔴 Critical
timePerformance/timePerformanceSynccreate throwawayPerformanceTrackerinstances — accumulated metrics are inaccessible, and timing is broken due to fixedcurrentTime.Each call to
performanceTracker()constructs a fresh instance. WhilestopTimer()logs individual events to analytics, the internalperformanceMetricsstate is discarded, makinggetPerformanceStats()andlogPerformanceSummary()unreachable for any tracker obtained via these wrappers.More critically, line 267 defines
currentTimeas a fixedLongvalue evaluated once at module load, not a dynamic function. This causes all duration calculations (currentTime - startTime) to be zero or near-zero, rendering the entire timing system broken—affecting not only these convenience functions but any directPerformanceTrackerusage.Fix required:
- Change line 267 from
private val currentTime = ...to a computed property or function that returns the current time on each access.- Consider whether
timePerformance/timePerformanceSyncshould accept an optional tracker parameter to support accumulated stats, or document that they are single-shot only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/PerformanceTracker.kt` around lines 270 - 284, The convenience wrappers timePerformance and timePerformanceSync call performanceTracker() which creates a fresh PerformanceTracker each time (losing accumulated metrics from performanceMetrics and making getPerformanceStats()/logPerformanceSummary() unusable), and the module-level private val currentTime is evaluated once at load causing zero durations; change currentTime into a function or computed property (e.g., fun currentTime() = System.nanoTime()/SystemClock.elapsedRealtimeMillis() as appropriate) so each measurement uses the real current time, and either update timePerformance/timePerformanceSync to accept an optional PerformanceTracker parameter (or reuse a shared instance) so callers can accumulate metrics instead of always getting a throwaway tracker; ensure stopTimer(), performanceMetrics, getPerformanceStats(), and logPerformanceSummary() continue to operate on the same tracker instance when provided.
core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/PerformanceTracker.kt
Outdated
Show resolved
Hide resolved
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/FileKitUtil.kt
Show resolved
Hide resolved
...esignsystem/src/jsMain/kotlin/com/mifos/core/designsystem/component/MifosPermissionBox.js.kt
Show resolved
Hide resolved
...tem/src/wasmJsMain/kotlin/com/mifos/core/designsystem/component/MifosPermissionBox.wasmJs.kt
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/utils/PlatformFile.kt
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/utils/PlatformFile.kt
Show resolved
Hide resolved
feature/client/src/jsMain/kotlin/com/mifos/feature/client/utils/PdfViewer.js.kt
Show resolved
Hide resolved
feature/client/src/jsMain/kotlin/com/mifos/feature/client/utils/PhoneNumberUtil.js.kt
Show resolved
Hide resolved
...-tracking/src/wasmJsMain/kotlin/com/mifos/feature/path/tracking/PathTrackingScreen.wasmJs.kt
Show resolved
Hide resolved
|
@Arinyadav1 Can you please address the suggestions by @coderabbitai (relevant to your PR)? Please notify me after you are done. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt (2)
520-524:⚠️ Potential issue | 🟡 MinorDoc example references
@TypeConverters(plural) which has been removed.The
TypeConverterKDoc at line 522–523 shows@TypeConverters(DateConverters::class), but theTypeConverters(plural) annotation was removed in this change set. The example will confuse users since only@TypeConverter(singular, method-level) exists now.Proposed fix — remove or update the stale reference
- * // Register in database - * `@Database`(...) - * `@TypeConverters`(DateConverters::class) - * abstract class MyDatabase : RoomDatabase() + * // Register converters according to your platform's database configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt` around lines 520 - 524, The KDoc example still shows the removed plural annotation `@TypeConverters(DateConverters::class)` which will confuse readers; update the example in Room.kt to remove that line and instead reference the correct singular usage: mention that conversion methods should be annotated with `@TypeConverter` inside a `DateConverters` class (or similar) and then used by the database. Locate the example block (the lines showing `@Database(...)` and `@TypeConverters(DateConverters::class)`) and delete or replace the `@TypeConverters(...)` line with a short note like “implement conversion methods in DateConverters annotated with `@TypeConverter`” referencing the `DateConverters` type and the `@TypeConverter` annotation.
305-310: 🛠️ Refactor suggestion | 🟠 MajorMigrate
Upsertannotation from@Suppress("NO_ACTUAL_FOR_EXPECT")to@OptionalExpectation.
Upsertis inconsistent withInsert,Update, andDelete, which all use@OptIn(ExperimentalMultiplatform::class)+@OptionalExpectation. Since no platform-specific actual implementations ofUpsertexist, it must be updated to match the pattern used by its siblings to ensure consistent behavior across targets.Proposed fix
-@Suppress("NO_ACTUAL_FOR_EXPECT") +@OptIn(ExperimentalMultiplatform::class) +@OptionalExpectation `@Target`(AnnotationTarget.FUNCTION) `@Retention`(AnnotationRetention.BINARY) expect annotation class Upsert( val entity: KClass<*>, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt` around lines 305 - 310, The Upsert annotation is incorrectly marked with `@Suppress`("NO_ACTUAL_FOR_EXPECT") instead of the module's pattern; replace the `@Suppress` and its import with `@OptIn`(ExperimentalMultiplatform::class) and annotate Upsert with `@OptionalExpectation` so it matches Insert/Update/Delete; update the file-level annotations/imports for ExperimentalMultiplatform and OptionalExpectation and keep the expect annotation class Upsert(val entity: KClass<*>) unchanged.
🧹 Nitpick comments (1)
core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt (1)
153-163:ForeignKeyis missing@OptIn(ExperimentalMultiplatform::class)/@OptionalExpectation— intentional but worth a brief inline comment.Unlike the directly-applied annotations,
ForeignKeycan't use@OptionalExpectationbecause it's referenced as a type inEntity's constructor (val foreignKeys: Array<ForeignKey>), so it must be resolvable on every target. The file-level@Suppress("KotlinNoActualForExpect")covers it. A one-line comment here (and onIndex,Junction,AutoMigration) explaining why this subset doesn't carry@OptionalExpectationwould save future readers from treating it as an oversight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt` around lines 153 - 163, Add a one-line inline comment above the expect annotation class ForeignKey (and similarly above Index, Junction, AutoMigration) explaining that these annotation types intentionally do not carry `@OptionalExpectation/`@OptIn(ExperimentalMultiplatform::class) because they are referenced as types (e.g., Entity.val foreignKeys: Array<ForeignKey>) and therefore must be resolvable on every target; reference the existing file-level `@Suppress`("KotlinNoActualForExpect") as the mechanism that covers them to prevent future maintainers from treating the omission as an oversight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt`:
- Around line 128-137: The doc example in Room.kt incorrectly references
ForeignKey.CASCADE; update the example to use ForeignKeyAction.CASCADE (the
cascade constant is defined on ForeignKeyAction) so the annotation reads using
ForeignKeyAction.CASCADE for onDelete; ensure the import/usage in the example
matches the symbol ForeignKeyAction and not ForeignKey.
---
Outside diff comments:
In
`@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt`:
- Around line 520-524: The KDoc example still shows the removed plural
annotation `@TypeConverters(DateConverters::class)` which will confuse readers;
update the example in Room.kt to remove that line and instead reference the
correct singular usage: mention that conversion methods should be annotated with
`@TypeConverter` inside a `DateConverters` class (or similar) and then used by
the database. Locate the example block (the lines showing `@Database(...)` and
`@TypeConverters(DateConverters::class)`) and delete or replace the
`@TypeConverters(...)` line with a short note like “implement conversion methods
in DateConverters annotated with `@TypeConverter`” referencing the
`DateConverters` type and the `@TypeConverter` annotation.
- Around line 305-310: The Upsert annotation is incorrectly marked with
`@Suppress`("NO_ACTUAL_FOR_EXPECT") instead of the module's pattern; replace the
`@Suppress` and its import with `@OptIn`(ExperimentalMultiplatform::class) and
annotate Upsert with `@OptionalExpectation` so it matches Insert/Update/Delete;
update the file-level annotations/imports for ExperimentalMultiplatform and
OptionalExpectation and keep the expect annotation class Upsert(val entity:
KClass<*>) unchanged.
---
Nitpick comments:
In
`@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt`:
- Around line 153-163: Add a one-line inline comment above the expect annotation
class ForeignKey (and similarly above Index, Junction, AutoMigration) explaining
that these annotation types intentionally do not carry
`@OptionalExpectation/`@OptIn(ExperimentalMultiplatform::class) because they are
referenced as types (e.g., Entity.val foreignKeys: Array<ForeignKey>) and
therefore must be resolvable on every target; reference the existing file-level
`@Suppress`("KotlinNoActualForExpect") as the mechanism that covers them to
prevent future maintainers from treating the omission as an oversight.
core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt (2)
305-310:Upsertis the only function-levelexpectannotation still using the old@Suppress("NO_ACTUAL_FOR_EXPECT")pattern.Every other directly-applicable
expect annotation classwith function targets in this file (Query,Insert,Update,Delete) has been migrated to@OptIn(ExperimentalMultiplatform::class)+@OptionalExpectation. No actualUpsertimplementation exists in the codebase, making the modern pattern semantically correct and safe to adopt.Migrating
Upserteliminates this inconsistency and allows the file-level@file:Suppress("KotlinNoActualForExpect")(line 10) to be removed.♻️ Proposed refactor
-@Suppress("NO_ACTUAL_FOR_EXPECT") +@OptIn(ExperimentalMultiplatform::class) +@OptionalExpectation `@Target`(AnnotationTarget.FUNCTION) `@Retention`(AnnotationRetention.BINARY) expect annotation class Upsert( val entity: KClass<*>, )Remove the file-level suppression:
-@file:Suppress("KotlinNoActualForExpect")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt` around lines 305 - 310, The Upsert expect annotation should be migrated from using the old `@Suppress`("NO_ACTUAL_FOR_EXPECT") pattern to the modern optional-expectation approach used by Query/Insert/Update/Delete: annotate the expect annotation class Upsert with `@OptIn`(ExperimentalMultiplatform::class) and `@OptionalExpectation`, remove the per-function `@Suppress`, and then you can drop the file-level `@file`:Suppress("KotlinNoActualForExpect"); update the declaration of Upsert (the expect annotation class with val entity: KClass<*>) to match the other function-level expect annotations so the codebase is consistent and compiles without an actual implementation.
153-163: Add default values toForeignKeyannotation parameters to align with Room's API.The
onDelete,onUpdate, anddeferredparameters should have defaults matchingandroidx.room.ForeignKey:onDelete = ForeignKeyAction.NO_ACTION,onUpdate = ForeignKeyAction.NO_ACTION, anddeferred = false. This aligns with Room's actual API design and provides a cleaner annotation interface for potential new use cases.🛠️ Proposed fix
expect annotation class ForeignKey( val entity: KClass<*>, val parentColumns: Array<String>, val childColumns: Array<String>, - val onDelete: Int, - val onUpdate: Int, - val deferred: Boolean, + val onDelete: Int = ForeignKeyAction.NO_ACTION, + val onUpdate: Int = ForeignKeyAction.NO_ACTION, + val deferred: Boolean = false, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt` around lines 153 - 163, The ForeignKey expect-annotation is missing defaults for onDelete, onUpdate, and deferred; update the declaration of expect annotation class ForeignKey to set onDelete = androidx.room.ForeignKeyAction.NO_ACTION, onUpdate = androidx.room.ForeignKeyAction.NO_ACTION, and deferred = false so it matches Room's API defaults (leave entity, parentColumns, childColumns required). Ensure you reference the ForeignKey annotation and its parameters (onDelete, onUpdate, deferred) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt`:
- Around line 128-137: The example was corrected to use ForeignKeyAction.CASCADE
instead of the incorrect ForeignKey.CASCADE; verify and replace any remaining
usages of ForeignKey.CASCADE with ForeignKeyAction.CASCADE (e.g., in the `@Entity`
foreignKeys block referencing ForeignKey, parentColumns, childColumns and
onDelete) so all examples consistently reference ForeignKeyAction.CASCADE.
---
Nitpick comments:
In
`@core-base/database/src/commonMain/kotlin/template/core/base/database/Room.kt`:
- Around line 305-310: The Upsert expect annotation should be migrated from
using the old `@Suppress`("NO_ACTUAL_FOR_EXPECT") pattern to the modern
optional-expectation approach used by Query/Insert/Update/Delete: annotate the
expect annotation class Upsert with `@OptIn`(ExperimentalMultiplatform::class) and
`@OptionalExpectation`, remove the per-function `@Suppress`, and then you can drop
the file-level `@file`:Suppress("KotlinNoActualForExpect"); update the declaration
of Upsert (the expect annotation class with val entity: KClass<*>) to match the
other function-level expect annotations so the codebase is consistent and
compiles without an actual implementation.
- Around line 153-163: The ForeignKey expect-annotation is missing defaults for
onDelete, onUpdate, and deferred; update the declaration of expect annotation
class ForeignKey to set onDelete = androidx.room.ForeignKeyAction.NO_ACTION,
onUpdate = androidx.room.ForeignKeyAction.NO_ACTION, and deferred = false so it
matches Room's API defaults (leave entity, parentColumns, childColumns
required). Ensure you reference the ForeignKey annotation and its parameters
(onDelete, onUpdate, deferred) when making the change.
|
@biplab1 please do the final review |
|
@biplab1 please review this one |
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (2)
1288-1307:⚠️ Potential issue | 🟡 Minor
isAddressTypeIdValidboundary check allows unselected address type (id = 0) to pass silently.The guard is
addressTypeId < 0, butselectedAddressTypeIdis initialised to0(line 313). When the address section is enabled and the user never picks an address type,isAddressTypeIdValid(0, …)returnstrue, submitting an address payload withaddressTypeId = 0. The check should be<= 0.🛡️ Proposed fix
- addressTypeId < 0 -> { + addressTypeId <= 0 -> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 1288 - 1307, The boundary check in isAddressTypeIdValid incorrectly allows addressTypeId == 0 (the unselected default) to pass; update the guard in isAddressTypeIdValid to treat non-positive ids as invalid (change the condition from addressTypeId < 0 to addressTypeId <= 0) so that selectedAddressTypeId initialized to 0 will trigger the snackbar via scope.launch and return false when no type is chosen.
353-369:⚠️ Potential issue | 🟠 MajorCommenting out
selectedImagePathassignment silently breaks both image preview and upload.Two regressions stem from this change:
Image preview always shows placeholder —
selectedImagePathremainsnullafter image selection, soClientImageSection(line 471) always renders the placeholder image instead of the selected image.Image upload is never triggered —
setFileForUpload.invoke(selectedImagePath)(line 795) always receivesnull, so the lambda at lines 222–225 never setscreateClientWithImage = true. TheSetClientIdstate handler (lines 233–239) then always callsnavigateBack()instead ofuploadImage(uiState.id), even thoughviewmodel.updateSelectedImage(file)has stored the file in the ViewModel.The ViewModel holds the picked file but the UI layer has no way to observe or use it.
Suggested approach:
Replace the
String?preview state with aPlatformFile?state variable:Proposed fix sketch
- var selectedImagePath by rememberSaveable { mutableStateOf<String?>(null) } + var selectedPlatformFile by remember { mutableStateOf<PlatformFile?>(null) }In the launchers:
val galleryLauncher = rememberFilePickerLauncher(type = FileKitType.Image) { file -> file?.let { -// TODO: path not support in kmp all targets -// selectedImagePath = file.path onImageSelected(file) + selectedPlatformFile = file } }Pass
selectedPlatformFiletoClientImageSectionfor preview (filekit-coil is already available in the project and can integrate with Coil 3'srememberAsyncImagePainter). For the upload trigger, derive it from the presence of a file rather than from a path:- setFileForUpload = { filePath -> - filePath?.let { createClientWithImage = true } - }, + setFileForUpload = { _ -> + if (selectedPlatformFile != null) createClientWithImage = true + },When removing an image (line 425), also reset
selectedPlatformFile = null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` around lines 353 - 369, The UI stopped showing previews and triggering uploads because selectedImagePath (String?) was commented out and UI no longer observes the picked file from the gallery/camera; fix by replacing the String? preview state with a PlatformFile? (e.g., selectedPlatformFile) and update the galleryLauncher and cameraLauncher callbacks (where onImageSelected(file) is called) to set selectedPlatformFile = file and still call viewmodel.updateSelectedImage(file); pass selectedPlatformFile into ClientImageSection for preview rendering; change the upload trigger (setFileForUpload invocation) to derive from selectedPlatformFile presence (not a path string) and ensure the image-remove handler also sets selectedPlatformFile = null so preview and upload flow work end-to-end.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)
89-89:MaterialThemeshould be replaced withKptThemeper project convention.Several usages across this file (
colorScheme.outlineat line 966,colorScheme.surface/colorScheme.secondaryat lines 991, 1010, 1021, 1031, andtypography.bodyLargeat lines 1003, 1015, 1025, 1035) referenceMaterialThemedirectly. The project convention is to useKptThemeinstead.♻️ Import to add
-import androidx.compose.material3.MaterialTheme +import template.core.base.designsystem.theme.KptThemeThen replace all
MaterialTheme.colorScheme.*/MaterialTheme.typography.*references withKptTheme.colorScheme.*/KptTheme.typography.*accordingly.Based on learnings from PR
#2610: In Kotlin files under the android-client module, replaceMaterialThemereferences withKptThemeimported fromtemplate.core.base.designsystem.theme.KptTheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt` at line 89, Replace the MaterialTheme import and usages with the project's KptTheme: remove the androidx.compose.material3.MaterialTheme import and add import template.core.base.designsystem.theme.KptTheme, then update all occurrences of MaterialTheme.colorScheme.outline, MaterialTheme.colorScheme.surface, MaterialTheme.colorScheme.secondary and MaterialTheme.typography.bodyLarge in CreateNewClientScreen (e.g., the places referencing colorScheme.outline, surface, secondary and typography.bodyLarge) to use KptTheme.colorScheme.* and KptTheme.typography.bodyLarge respectively so the file follows the android-client convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 1288-1307: The boundary check in isAddressTypeIdValid incorrectly
allows addressTypeId == 0 (the unselected default) to pass; update the guard in
isAddressTypeIdValid to treat non-positive ids as invalid (change the condition
from addressTypeId < 0 to addressTypeId <= 0) so that selectedAddressTypeId
initialized to 0 will trigger the snackbar via scope.launch and return false
when no type is chosen.
- Around line 353-369: The UI stopped showing previews and triggering uploads
because selectedImagePath (String?) was commented out and UI no longer observes
the picked file from the gallery/camera; fix by replacing the String? preview
state with a PlatformFile? (e.g., selectedPlatformFile) and update the
galleryLauncher and cameraLauncher callbacks (where onImageSelected(file) is
called) to set selectedPlatformFile = file and still call
viewmodel.updateSelectedImage(file); pass selectedPlatformFile into
ClientImageSection for preview rendering; change the upload trigger
(setFileForUpload invocation) to derive from selectedPlatformFile presence (not
a path string) and ensure the image-remove handler also sets
selectedPlatformFile = null so preview and upload flow work end-to-end.
---
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Line 89: Replace the MaterialTheme import and usages with the project's
KptTheme: remove the androidx.compose.material3.MaterialTheme import and add
import template.core.base.designsystem.theme.KptTheme, then update all
occurrences of MaterialTheme.colorScheme.outline,
MaterialTheme.colorScheme.surface, MaterialTheme.colorScheme.secondary and
MaterialTheme.typography.bodyLarge in CreateNewClientScreen (e.g., the places
referencing colorScheme.outline, surface, secondary and typography.bodyLarge) to
use KptTheme.colorScheme.* and KptTheme.typography.bodyLarge respectively so the
file follows the android-client convention.
2 participants
Fixes - Jira-#644
Summary by CodeRabbit
New Features
Bug Fixes
Chores