refactor(feature): replace MaterialTheme with kptTheme#2610
refactor(feature): replace MaterialTheme with kptTheme#2610kartikey004 wants to merge 1 commit intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughUpdates dependencies (Compose Material3 adaptive and androidx.window to 1.3.x), adds new design token values, exposes the designsystem project API, and migrates UI theming across many screens from MaterialTheme/DesignToken to a centralized KptTheme with spacing/typography token replacements and minor composable signature updates. Changes
Files with signature/type changes or API-affecting edits are small in number; the vast majority are presentation migrations. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major 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 (12)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (1)
556-560:⚠️ Potential issue | 🟠 MajorBug: Row click toggles the wrong checkbox state.
Line 559 passes
!state.isCheckedEqualAmortizationtoOnInterestPartialPeriodCheckChange, but it should use!state.isCheckedInterestPartialPeriod. TheCheckboxon line 564 correctly readsisCheckedInterestPartialPeriod, so clicking the row and clicking the checkbox produce inconsistent behavior.This appears to be a pre-existing bug, but since you're touching this file it's worth fixing now.
Proposed fix
Row( Modifier.fillMaxWidth() .clickable { - onAction(NewLoanAccountAction.OnInterestPartialPeriodCheckChange(!state.isCheckedEqualAmortization)) + onAction(NewLoanAccountAction.OnInterestPartialPeriodCheckChange(!state.isCheckedInterestPartialPeriod)) },feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/newIndividualCollectionSheet/NewIndividualCollectionSheetScreen.kt (1)
326-397:⚠️ Potential issue | 🟡 MinorPre-existing:
CollectionSheetDialogContentcreates a nestedMifosBottomSheet.
CollectionSheetDialogContent(line 326) wraps its content in aMifosBottomSheet, but it's already called inside aMifosBottomSheetat line 184–206. This results in a double-nested bottom sheet. The innerCollectionSheetDialogContentshould likely just emit itsColumncontent directly, leaving the sheet management to the caller.Not introduced by this PR, but worth addressing.
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/syncSurvey/SyncSurveysDialog.kt (1)
441-451:⚠️ Potential issue | 🟡 MinorPreview not wrapped in
KptTheme.
SyncSurveysDialogContent(and transitivelySyncSurveyButton) readsKptTheme.spacing,KptTheme.colorScheme, andKptTheme.shapes. Without wrapping in aKptTheme { … }block, previews will either crash or render with unexpected defaults.Suggested fix
`@Composable` `@DevicePreview` private fun SyncSurveysDialogPreview() { - Column { - SyncSurveysDialog(uiState = SyncSurveysDialogUiState.DismissDialog, closeDialog = {}) - SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowError("Error"), closeDialog = {}) - SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowProgressbar, closeDialog = {}) - SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowSurveysSyncSuccessfully, closeDialog = {}) - SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowSyncedFailedSurveys(1), closeDialog = {}) + KptTheme { + Column { + SyncSurveysDialog(uiState = SyncSurveysDialogUiState.DismissDialog, closeDialog = {}) + SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowError("Error"), closeDialog = {}) + SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowProgressbar, closeDialog = {}) + SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowSurveysSyncSuccessfully, closeDialog = {}) + SyncSurveysDialog(uiState = SyncSurveysDialogUiState.ShowSyncedFailedSurveys(1), closeDialog = {}) + } } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSurveyQuestion/SurveyQuestionScreen.kt (1)
326-340:⚠️ Potential issue | 🟡 MinorWrap
PreviewSurveyQuestionScreenwithKptThemeto provide required theme locals.
SurveyQuestionScreenaccessesKptTheme.typography,KptTheme.colorScheme, andKptTheme.spacingthroughout its implementation, which are provided viaCompositionLocal. The preview will crash with an unresolved composition local error without the wrapper.Proposed fix
`@Composable` `@DevicePreview` private fun PreviewSurveyQuestionScreen() { + KptTheme { SurveyQuestionScreen( uiState = SurveySubmitUiState.Initial, navigateBack = { }, questionNumber = 1, currentQuestionData = mutableListOf(), currentOptionsData = mutableListOf(), currentScoreCardData = listOf(), gotoNextQuestion = { }, showSubmitScreen = false, submitSurvey = { }, ) + } }feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanCharge/LoanChargeScreen.kt (1)
242-256:⚠️ Potential issue | 🟡 MinorPreview not wrapped in
KptTheme.Since the screen now relies on
KptThemefor spacing, colors, typography, shapes, and elevation, the preview should be wrapped in theKptTheme {}composable to ensure it renders correctly and doesn't crash due to missingCompositionLocalproviders.Proposed fix
`@Preview` `@Composable` private fun LoanChargeScreenPreview( `@PreviewParameter`(LoanChargeUiStateProvider::class) state: LoanChargeUiState, ) { + KptTheme { LoanChargeScreen( loanAccountNumber = 1, state = state, onChargeCreated = {}, onBackPressed = {}, onRetry = {}, refreshState = false, onRefresh = {}, ) + } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (1)
168-200:⚠️ Potential issue | 🟠 MajorBug: Currency symbol prepended to "Not Available" fallback text.
When
loan.currency?.displaySymbolis non-empty (e.g."$"), the fallback produces strings like"$Not Available"fororiginalLoan,amountPaid, andloanBalance. The symbol should only be prepended when an actual numeric value is available.🐛 Proposed fix (showing originalLoan as example — apply same pattern to amountPaid and loanBalance)
- originalLoan = symbol + ( - (loan.originalLoan ?: "Not Available").toString() - ), + originalLoan = loan.originalLoan + ?.let { symbol + it.toString() } + ?: "Not Available",For
amountPaid/loanBalance, guard the symbol similarly:- amountPaid = symbol + ( - ( - if (loan.status?.pendingApproval == true) { - "Not Available" - } else { - ( - loan.amountPaid - ?: 0.0 - ).toString() - } - ) - ), + amountPaid = if (loan.status?.pendingApproval == true) { + "Not Available" + } else { + symbol + (loan.amountPaid ?: 0.0).toString() + },feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (1)
240-271:⚠️ Potential issue | 🟠 MajorColor contrast mismatch: content colors don't match the container's semantic role.
The TopAppBar uses
KptTheme.colorScheme.secondaryas its container color, but the title text usesonBackground(line 246) and icons useonSurface(lines 257, 271). For correct contrast on asecondarybackground, these should useonSecondary.🎨 Proposed fix for semantic color consistency
title = { Text( text = "${selectedItems.size} selected", style = KptTheme.typography.titleLarge.copy( - color = KptTheme.colorScheme.onBackground, + color = KptTheme.colorScheme.onSecondary, ), ) }, navigationIcon = { IconButton( onClick = resetSelectionMode, ) { Icon( imageVector = MifosIcons.Close, contentDescription = "Close", - tint = KptTheme.colorScheme.onSurface, + tint = KptTheme.colorScheme.onSecondary, ) } }, actions = { IconButton( onClick = { syncClicked() resetSelectionMode() }, ) { Icon( imageVector = MifosIcons.Sync, contentDescription = "Sync", - tint = KptTheme.colorScheme.onSurface, + tint = KptTheme.colorScheme.onSecondary, ) } },feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/groupLoanAccount/GroupLoanAccountScreen.kt (1)
296-300:⚠️ Potential issue | 🟠 MajorPre-existing bug: dismiss button closes wrong date picker.
Line 298 sets
showSubmissionDatePicker = falseinside the disbursement date picker's dismiss button. It should setshowDisbursementDatePicker = false. This is not introduced by this PR but is worth fixing while you're touching this file.🐛 Proposed fix
dismissButton = { TextButton( onClick = { - showSubmissionDatePicker = false + showDisbursementDatePicker = false }, ) { Text(stringResource(Res.string.feature_loan_cancel)) } },feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/createNewGroup/CreateNewGroupScreen.kt (1)
342-362:⚠️ Potential issue | 🟡 Minor
AnimatedVisibilitycontent children overlap — wrap in aColumn.
AnimatedVisibilityuses aBoxlayout internally. TheSpacer(line 353) andMifosDatePickerTextField(line 355) are siblings in aBox, so they'll overlap rather than stack vertically. The Spacer has no visible effect here.Proposed fix: wrap content in a Column
AnimatedVisibility( visible = isActive, enter = slideInVertically { with(density) { -40.dp.roundToPx() } } + expandVertically( expandFrom = Alignment.Top, ) + fadeIn( initialAlpha = 0.3f, ), exit = slideOutVertically() + shrinkVertically() + fadeOut(), ) { + Column { Spacer(modifier = Modifier.height(KptTheme.spacing.md)) MifosDatePickerTextField( value = DateHelper.getDateAsStringFromLong(activationDate), label = stringResource(Res.string.feature_groups_activation_date), openDatePicker = { activationDatePicker = true }, ) + } }feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt (1)
430-442:⚠️ Potential issue | 🟡 Minor
Color.Greenis still hardcoded while other transaction colors use theme tokens.The withdrawal case uses
KptTheme.colorScheme.errorand the default usesKptTheme.colorScheme.onSurface, but the deposit case still uses a rawColor.Green. This is inconsistent with the KptTheme migration. Consider using a theme-appropriate color (e.g., a success/tertiary token or a custom semantic color from the design system).feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupDetails/GroupDetailsScreen.kt (1)
594-596:⚠️ Potential issue | 🟡 MinorInconsistent icon button sizing between loan and savings expandable cards.
MifosLoanAccountExpendableCard(line 432) usesDesignToken.sizes.iconMediumfor theIconButtonsize, butMifosSavingsAccountExpendableCarduses a hardcoded24.dp. These should match for visual consistency.Proposed fix
IconButton( modifier = Modifier - .size(24.dp), + .size(DesignToken.sizes.iconMedium), onClick = { expendableState = !expendableState },feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
835-855:⚠️ Potential issue | 🟡 MinorReplace
MifosThemewithKptThemein the preview wrapper.The preview uses
MifosTheme(line 840), but all composables read fromKptTheme.colorScheme,KptTheme.typography,KptTheme.spacing, andKptTheme.shapes. TheMifosThemefunction does not provide theKptThemeCompositionLocals; instead, it provides onlyDesignTokenlocals viaDesignTokenTheme. This causes the preview to render with default theme values rather than the actualMifosThemecolors, making the preview inconsistent with runtime appearance.Wrap the preview content in
KptTheme { ... }(or nest it withinMifosThemeappropriately ifMifosThemeshould provideKptThemeinternally).
🤖 Fix all issues with AI agents
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt`:
- Around line 98-109: The new raw-valued tokens (dp1, dp2, dp30, dp40, dp44,
dp46, dp50, dp52, dp80, dp160, dp164, dp300) break the semantic token system;
replace each dp* with a descriptive semantic token name that indicates its
intent (for example use names like borderWidth, sectionGap, dialogWidth,
iconSpacing, avatarLarge) or remove the token and use the literal dp value at
the call site if it’s a one-off; update the DesignToken declarations (the vals
named dp1...dp300) to match existing semantic patterns (e.g., extraSmall,
medium, large, iconAverage, avatarMedium) and ensure no duplicates of existing
semantic tokens remain.
- Around line 328-337: The new AppSizes raw dp tokens create duplicates of
existing semantic tokens; remove the redundant properties dp20, dp48, dp72,
dp120, and dp128 from DesignToken/AppSizes and update any call sites to use the
semantic tokens iconAverage, avatarMedium, profile, cardMinHeight, and
avatarLargeLarge respectively (leave other dp* entries intact); ensure no
references remain to the removed dp properties and run a build to catch any
lingering usages.
- Line 159: The AppPadding property dp40 uses a raw numeric name; replace it
with a semantic name or remove it so callers use 40.dp directly. Locate the
AppPadding data class (property dp40) in DesignToken.kt and either (a) rename
dp40 to a descriptive semantic identifier that reflects its usage (e.g.,
cardContentPadding, sectionInset) and update all usages, or (b) delete the dp40
field and refactor call sites to use 40.dp inline where that padding is
required.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt`:
- Around line 220-223: The Box currently uses heightIn(max =
KptTheme.spacing.xs) which collapses the bottom sheet (xs = 4.dp) and clips its
Column content (24.dp padding lg, 32.dp spacing xl, title Text, LazyColumn,
buttons); remove or replace this constraint with an appropriate size (e.g., no
max constraint, heightIn(min = ...), or a larger token like
KptTheme.spacing.xl/xxl or Modifier.fillMaxHeight(fraction)) inside
ChargesScreen to allow the Column and LazyColumn to layout correctly—locate the
Box that wraps the Column and change the heightIn usage accordingly.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt`:
- Around line 303-321: MifosRowCard is styling its container using MaterialTheme
color tokens (e.g., MaterialTheme.colorScheme.surfaceBright and
MaterialTheme.colorScheme.onSurface) while callers (like where TextUtil is
passed with KptTheme.typography.bodySmall and KptTheme.colorScheme.secondary)
expect KptTheme to control appearance; update MifosRowCard to consume KptTheme
tokens instead of MaterialTheme (replace references to
MaterialTheme.colorScheme.* and any MaterialTheme typography usages with
KptTheme.colorScheme.* / KptTheme.typography.*), and audit other frequently used
components mentioned (PrintTextUtil, MifosUserSignature, MifosTwoButtonRow,
MifosTabRow, MifosTopAppBar) to follow the same pattern so containers and text
use the same KptTheme tokens for consistent rendering.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientGeneral/ClientProfileGeneralScreen.kt`:
- Around line 280-283: The current construction of a new TextStyle only copies
fontStyle and loses other typography properties from
KptTheme.typography.labelMedium; update the textStyle usage so it preserves the
full labelMedium style by calling KptTheme.typography.labelMedium.copy(...) and
override color (and optionally fontStyle) instead of creating a fresh TextStyle,
making the change where textStyle is defined in ClientProfileGeneralScreen.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt`:
- Around line 400-401: In ClientIdentitiesAddUpdateScreen, avoid
force-unwrapping state.documentImageFile when rendering PdfPreview: check that
state.documentImageFile is non-null before calling PdfPreview (e.g., use a
guard/early return or state.documentImageFile?.let { PdfPreview(it,
Modifier.matchParentSize()) } or add an explicit condition alongside
state.fileExtension == "pdf"); ensure the null-safe check surrounds the
PdfPreview call so no NPE can occur.
- Around line 249-251: The Column uses a fresh Modifier.fillMaxSize(),
discarding the Modifier passed in (which may contain Modifier.weight(1f));
replace Modifier.fillMaxSize() with modifier.fillMaxSize() (or
modifier.then(Modifier.fillMaxSize())) in the composables
ClientIdentitiesAddUpdateScreen, ClientIdentifiersAddUpdateDocument, and
ClientIdentifiersDocumentPreview so the incoming modifier (e.g., weight) is
preserved and the fill behavior is applied on top of it.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt`:
- Line 325: The code mixes KptTheme and DesignToken spacing (e.g.,
PaddingValues(top = KptTheme.spacing.sm, bottom = DesignToken.spacing.dp80));
reconcile by using KptTheme spacing consistently (replace
DesignToken.spacing.dp80 with an appropriate KptTheme spacing token such as
KptTheme.spacing.xl or add the equivalent token to KptTheme) while keeping size
values that only exist on DesignToken (DesignToken.sizes.*). In
SelectionModeTopAppBar change tints that use onBackground/onSurface to
onSecondary for text and icon colors when containerColor =
KptTheme.colorScheme.secondary so contrast is correct (update usages inside
SelectionModeTopAppBar where onBackground/onSurface are passed). Finally remove
the redundant modifier in the image/icon chain that uses
.size(DesignToken.sizes.logoSizeTopAppBar).fillMaxSize() — keep only the
intended constraint (.size(...) or .fillMaxSize()) to eliminate the no-op.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSurveyList/SurveyListScreen.kt`:
- Around line 174-178: The Box uses
Modifier.fillMaxWidth().size(DesignToken.sizes.dp5) which causes a 5×5dp square;
change the sizing to use height instead so the Box remains full-width: replace
the .size(...) call on the Box's Modifier with .height(DesignToken.sizes.dp5)
(keep Modifier.fillMaxWidth() and the background color
KptTheme.colorScheme.primary intact) to produce a full-width colored strip.
In
`@feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/individualCollectionSheetDetails/IndividualCollectionSheetDetailsScreen.kt`:
- Around line 258-272: The code uses the loop index from itemsIndexed (variable
index) to access client.loans via client.loans?.get(index), which can throw
IndexOutOfBounds when a client has fewer loans than their position; replace
these unsafe indexed accesses (client.loans?.get(index) in
IndividualCollectionSheetDetailsScreen) with a safe accessor such as
client.loans?.firstOrNull() or client.loans?.getOrNull(0) (or iterate loans
directly if multiple expected), and update the Text bindings that read totalDue
and chargesDue to use the selectedLoan null-safe (?.) properties with a fallback
(e.g., toString or empty) to avoid crashes.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 486-492: Remove the inline migration comments that were left next
to the Modifier.padding calls inside the Column; locate the Column composable
that uses Modifier.padding(horizontal = KptTheme.spacing.lg) and
Modifier.padding(bottom = KptTheme.spacing.xxl) (and the
.navigationBarsPadding() call) and delete the comments like "// PADDING:
Replaced DesignToken.padding.large with KptTheme.spacing.lg" so only the padding
modifiers remain.
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt`:
- Around line 112-113: Remove the inline migration breadcrumb comments (e.g.,
"// PADDING: Replaced DesignToken.padding.large with KptTheme.spacing.lg") that
were added during migration; locate occurrences near the Column/Modifier usages
and other UI composables in DetailsPage (references:
Column(Modifier.fillMaxSize().padding(...)), KptTheme.spacing.lg) and delete
those comment lines so the code remains functionally identical but without the
noise.
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt`:
- Line 52: The import for KptTheme in InterestPage.kt is incorrect; replace the
line importing template.core.base.designsystem.theme.KptTheme with the correct
package import template.core.base.designsystem.KptTheme so the compiler can find
the KptTheme symbol used in InterestPage (look for usages of KptTheme in the
InterestPage composable/function and update the import accordingly).
In
`@feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountApproval/SavingsAccountApprovalScreen.kt`:
- Around line 140-143: The default parameter for SavingsAccountApprovalContent
embeds theme-dependent padding (Modifier.padding(horizontal =
KptTheme.spacing.sm)) which causes callers passing their own modifier to lose
that padding; change the signature to use the standard modifier: Modifier =
Modifier and move the .padding(horizontal = KptTheme.spacing.sm) call into the
composable body (apply it to the root layout/composable that currently uses the
modifier) so the passed-in modifier is preserved while still applying
KptTheme.spacing.sm inside SavingsAccountApprovalContent.
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt`:
- Around line 292-295: Replace the inconsistent use of
MaterialTheme.shapes.medium with KptTheme.shapes.medium in the composable where
the shape is set (currently using MaterialTheme.shapes.medium in the shape = ...
parameter) so it aligns with the refactor; leave the BorderStroke width as
DesignToken.spacing.dp1 since KptTheme.spacing doesn't provide a 1.dp token.
🟡 Minor comments (10)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt-35-35 (1)
35-35:⚠️ Potential issue | 🟡 MinorRemove migration tracking comments before merging
The inline comments marking the theming changes (lines 43, 50, 52, 55, and related lines) are migration artifacts that should be removed. These don't add value to the production code and clutter the implementation.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/ChargesPage.kt-36-36 (1)
36-36:⚠️ Potential issue | 🟡 MinorInconsistent design token usage:
DesignTokenused for sizing whileKptThemeused for colors.Line 75 uses
DesignToken.sizes.iconSmallfor the icon size, but the icon's tint already usesKptTheme.colorScheme.primary. For consistency, either keepDesignTokenfor all sizing throughout the file or migrate toKptTheme.spacing.*tokens (as done inTermsPage.ktwhereKptTheme.spacing.mdwas used for icon sizing).KptThemedoes not currently expose explicit size tokens—only spacing tokens.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt-303-303 (1)
303-303:⚠️ Potential issue | 🟡 MinorReplace
DesignToken.sizes.iconMinyMinywithKptTheme.spacing.smfor consistency with the KptTheme migration.Line 303 uses
DesignToken.sizes.iconMinyMiny(8.dp) while the rest of the file has migrated toKptTheme. The equivalent in the new theme system isKptTheme.spacing.sm(also 8.dp), which is the pattern used elsewhere in the codebase for migrating icon size tokens to spacing values.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientTransfer/ClientTransferScreen.kt-217-217 (1)
217-217:⚠️ Potential issue | 🟡 MinorInconsistent design token usage:
DesignToken.sizesshould be migrated toKptThemeor added to the KptTheme interface.Lines 217 and 239 still reference
DesignToken.sizes.iconAveragewhile the rest of the file usesKptThemefor colors, typography, spacing, and elevation. Currently,KptThemedoes not expose a sizes property—only spacing (xs, sm, md, lg, xl, xxl). Either add a sizes property to theKptThemeinterface to replaceDesignToken.sizesusages, or document why component sizes are intentionally kept separate from the KptTheme tokens.feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt-58-59 (1)
58-59:⚠️ Potential issue | 🟡 MinorUse
1.dpdirectly or unify theme token usage on line 59.This line mixes
DesignToken.spacing.dp1withKptTheme.colorScheme.primary. SinceKptTheme.spacinglacks a 1.dp equivalent token (it starts atxs = 4.dp), consider using the literal1.dpfor clarity, which is clearer for small fixed values than introducingDesignTokenin an otherwiseKptTheme-based file.feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInbox/CheckerInboxScreen.kt-88-88 (1)
88-88:⚠️ Potential issue | 🟡 MinorInconsistent design token usage:
DesignToken.spacing.dp52on line 368 should useKptTheme.spacing.*instead.The file imports both
DesignToken(fromcom.mifos.core.designsystem.theme) andKptTheme(fromtemplate.core.base.designsystem.theme). All spacing, typography, elevation, and color references throughout the file consistently useKptTheme.*, except line 368 which usesDesignToken.spacing.dp52. This should be aligned with the rest of the file to useKptTheme.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt-354-356 (1)
354-356:⚠️ Potential issue | 🟡 MinorRedundant modifier chain:
.size()followed by.fillMaxSize()is a no-op.In Compose,
.size(DesignToken.sizes.logoSizeTopAppBar)already fixes the element's dimensions. The subsequent.fillMaxSize()has no effect because the size constraint is already resolved. Remove one or the other.🔧 Proposed fix
Image( painter = painterResource(Res.drawable.feature_client_ic_dp_placeholder), contentDescription = null, modifier = Modifier - .size(DesignToken.sizes.logoSizeTopAppBar) - .fillMaxSize(), + .size(DesignToken.sizes.logoSizeTopAppBar), contentScale = ContentScale.Fit, colorFilter = ColorFilter.tint(KptTheme.colorScheme.onPrimaryContainer), )feature/document/src/commonMain/kotlin/com/mifos/feature/document/documentDialog/DocumentDialogScreen.kt-344-347 (1)
344-347:⚠️ Potential issue | 🟡 MinorHardcoded user-facing string should be extracted to a string resource.
The text
"Supported formats: xls,xlsx,pdf,doc,docx,png,jpeg,jpg."is hardcoded, while every other user-facing string in this file usesstringResource(Res.string.…). Extract this to a string resource for i18n consistency.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpdateDefaultAccount/ClientUpdateDefaultAccountScreen.kt-142-142 (1)
142-142:⚠️ Potential issue | 🟡 Minor
Spacerwith.padding()instead of.width()— unintended sizing.
Spacer(Modifier.padding(KptTheme.spacing.sm))applies padding on all four sides of a zero-size composable, resulting in a total width of2 × smin thisRow. This is likely meant to be a simple horizontal gap.Proposed fix
- Spacer(Modifier.padding(KptTheme.spacing.sm)) + Spacer(Modifier.width(KptTheme.spacing.sm))This requires adding
widthimport if not already present:import androidx.compose.foundation.layout.widthfeature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt-73-76 (1)
73-76:⚠️ Potential issue | 🟡 MinorHardcoded content description string — consider using a string resource for i18n.
"Navigate back"is a user-facing accessibility string. For consistency with the rest of the codebase (which usesstringResource), extract this to a string resource.
| val dp1: Dp = 1.dp, | ||
| val dp2: Dp = 2.dp, | ||
| val dp30: Dp = 30.dp, | ||
| val dp40: Dp = 40.dp, | ||
| val dp44: Dp = 44.dp, | ||
| val dp46: Dp = 46.dp, | ||
| val dp50: Dp = 50.dp, | ||
| val dp52: Dp = 52.dp, | ||
| val dp80: Dp = 80.dp, | ||
| val dp160: Dp = 160.dp, | ||
| val dp164: Dp = 164.dp, | ||
| val dp300: Dp = 300.dp, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Raw dp* naming undermines the semantic design token system.
The existing tokens use descriptive semantic names (extraSmall, medium, large, iconAverage, avatarMedium, etc.), but the new additions (dp1, dp2, dp30, dp40, …) are just raw dp values as names. This defeats the purpose of a token system — consumers gain no context about what the value represents or where it should be used.
Additionally, some values duplicate existing semantic tokens (e.g., dp80 in spacing vs potential semantic equivalents).
Consider either:
- Assigning semantic names that convey intent (e.g.,
borderWidth,sectionGap,dialogWidth), or - Using the values directly at the call site if they're truly one-off and don't merit a token.
🤖 Prompt for AI Agents
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt`
around lines 98 - 109, The new raw-valued tokens (dp1, dp2, dp30, dp40, dp44,
dp46, dp50, dp52, dp80, dp160, dp164, dp300) break the semantic token system;
replace each dp* with a descriptive semantic token name that indicates its
intent (for example use names like borderWidth, sectionGap, dialogWidth,
iconSpacing, avatarLarge) or remove the token and use the literal dp value at
the call site if it’s a one-off; update the DesignToken declarations (the vals
named dp1...dp300) to match existing semantic patterns (e.g., extraSmall,
medium, large, iconAverage, avatarMedium) and ensure no duplicates of existing
semantic tokens remain.
| val extraLargeIncreased: Dp = 32.dp, | ||
| val extraExtraLarge: Dp = 48.dp, | ||
| val full: Dp = 1000.dp, | ||
| val dp40: Dp = 40.dp, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same concern: dp40 in AppPadding follows the raw-naming anti-pattern.
If this value has a specific semantic purpose, name it accordingly (e.g., cardContentPadding, sectionInset). Otherwise, use 40.dp directly at the call site.
🤖 Prompt for AI Agents
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt`
at line 159, The AppPadding property dp40 uses a raw numeric name; replace it
with a semantic name or remove it so callers use 40.dp directly. Locate the
AppPadding data class (property dp40) in DesignToken.kt and either (a) rename
dp40 to a descriptive semantic identifier that reflects its usage (e.g.,
cardContentPadding, sectionInset) and update all usages, or (b) delete the dp40
field and refactor call sites to use 40.dp inline where that padding is
required.
| val dp5: Dp = 5.dp, | ||
| val dp18: Dp = 18.dp, | ||
| val dp20: Dp = 20.dp, | ||
| val dp42: Dp = 42.dp, | ||
| val dp48: Dp = 48.dp, | ||
| val dp72: Dp = 72.dp, | ||
| val dp100: Dp = 100.dp, | ||
| val dp120: Dp = 120.dp, | ||
| val dp128: Dp = 128.dp, | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Several new AppSizes tokens duplicate existing semantic tokens.
dp48(48.dp) duplicatesavatarMediumdp72(72.dp) duplicatesprofiledp120(120.dp) duplicatescardMinHeightdp128(128.dp) duplicatesavatarLargeLargedp20(20.dp) duplicatesiconAverage
Consumers should reference the existing semantic tokens instead of creating raw-dp aliases. This will lead to a fragmented token surface where the same value has multiple names.
🤖 Prompt for AI Agents
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt`
around lines 328 - 337, The new AppSizes raw dp tokens create duplicates of
existing semantic tokens; remove the redundant properties dp20, dp48, dp72,
dp120, and dp128 from DesignToken/AppSizes and update any call sites to use the
semantic tokens iconAverage, avatarMedium, profile, cardMinHeight, and
avatarLargeLarge respectively (leave other dp* entries intact); ensure no
references remain to the removed dp properties and run a build to catch any
lingering usages.
| Box( | ||
| modifier = Modifier | ||
| .heightIn(max = DesignToken.spacing.half), | ||
| .heightIn(max = KptTheme.spacing.xs), | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full context of the code snippet (lines 215-230)
find . -path "*/feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt" -type f | head -1 | xargs cat -n | sed -n '215,230p'Repository: openMF/android-client
Length of output: 798
🏁 Script executed:
# Find the KptTheme spacing definitions
find . -name "*.kt" -type f | xargs rg -l 'spacing.*xs|object.*KptTheme' | head -10Repository: openMF/android-client
Length of output: 1120
🏁 Script executed:
# Search for KptTheme.spacing definition
rg -n 'spacing\s*=' --type=kt -A3 -B3 | head -100Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Also check for 'val xs' in theme files
rg -n 'val\s+xs\s*=' --type=kt -B2 -A2 | head -50Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Find theme-related files
find . -name "*Theme*.kt" -o -name "*DesignToken*.kt" | head -20Repository: openMF/android-client
Length of output: 1417
🏁 Script executed:
# Search for spacing definition without relying on file type
rg -n "spacing\s*=" -A5 | grep -A5 "xs\|object.*spacing" | head -60Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Look at the full Box and Column structure to understand intent
sed -n '220,240p' feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.ktRepository: openMF/android-client
Length of output: 1053
🏁 Script executed:
# Search for KptTheme object definition
rg -n "object\s+KptTheme|class\s+KptTheme" -A20 | head -80Repository: openMF/android-client
Length of output: 8338
🏁 Script executed:
# Find KptSpacingImpl definition
rg -n "class\s+KptSpacingImpl|data class\s+KptSpacingImpl" -A30Repository: openMF/android-client
Length of output: 4581
🏁 Script executed:
# Also search for KptSpacing interface definition
rg -n "interface\s+KptSpacing|class\s+KptSpacing" -A30 | head -80Repository: openMF/android-client
Length of output: 10491
🏁 Script executed:
# Search in DesignToken file which was mentioned
cat ./core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt | head -200Repository: openMF/android-client
Length of output: 7411
heightIn(max = KptTheme.spacing.xs) will completely collapse the bottom sheet content.
KptTheme.spacing.xs resolves to 4.dp. Using it as the max height constraint on a Box containing a Column with 24.dp padding (lg), 32.dp spacing (xl) between items, a title Text, a LazyColumn of charges, and button actions will render the entire content invisible or severely clipped. This appears to be a token mapping error—the constraint should likely be a much larger dimension or removed entirely.
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt`
around lines 220 - 223, The Box currently uses heightIn(max =
KptTheme.spacing.xs) which collapses the bottom sheet (xs = 4.dp) and clips its
Column content (24.dp padding lg, 32.dp spacing xl, title Text, LazyColumn,
buttons); remove or replace this constraint with an appropriate size (e.g., no
max constraint, heightIn(min = ...), or a larger token like
KptTheme.spacing.xl/xxl or Modifier.fillMaxHeight(fraction)) inside
ChargesScreen to allow the Column and LazyColumn to layout correctly—locate the
Box that wraps the Column and change the heightIn usage accordingly.
...ommonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
| // PADDING: Replaced DesignToken.padding.large with KptTheme.spacing.lg | ||
| Column(Modifier.fillMaxSize().padding(bottom = KptTheme.spacing.lg)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove migration breadcrumb comments.
Lines 112, 119, 123, and 160 contain inline comments like // PADDING: Replaced DesignToken.padding.large with KptTheme.spacing.lg that document what was changed during the migration. These add noise and have no long-term value — please remove them before merging.
🧹 Proposed cleanup
- // PADDING: Replaced DesignToken.padding.large with KptTheme.spacing.lg
Column(Modifier.fillMaxSize().padding(bottom = KptTheme.spacing.lg)) {- // TYPOGRAPHY: Mapped to KptTheme
style = KptTheme.typography.titleMedium,- // SPACING: Replaced DesignToken.padding.large with KptTheme.spacing.lg
Spacer(Modifier.height(KptTheme.spacing.lg))- // SPACING: Replaced DesignToken.padding.large with KptTheme.spacing.lg
Spacer(Modifier.height(KptTheme.spacing.lg))Also applies to: 119-124, 123-124, 160-161
🤖 Prompt for AI Agents
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt`
around lines 112 - 113, Remove the inline migration breadcrumb comments (e.g.,
"// PADDING: Replaced DesignToken.padding.large with KptTheme.spacing.lg") that
were added during migration; locate occurrences near the Column/Modifier usages
and other UI composables in DetailsPage (references:
Column(Modifier.fillMaxSize().padding(...)), KptTheme.spacing.lg) and delete
those comment lines so the code remains functionally identical but without the
noise.
| import com.mifos.feature.recurringDeposit.newRecurringDepositAccount.RecurringAccountAction | ||
| import com.mifos.feature.recurringDeposit.newRecurringDepositAccount.RecurringAccountState | ||
| import org.jetbrains.compose.resources.stringResource | ||
| import template.core.base.designsystem.theme.KptTheme |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual package declaration of KptTheme
echo "=== Searching for KptTheme.kt definition ==="
fd "KptTheme\.kt" --type=f
echo ""
echo "=== Checking imports of KptTheme in the codebase ==="
rg -n "import.*KptTheme" --type=kt | head -30
echo ""
echo "=== Checking if 'template.core.base' is a standard package prefix ==="
rg -n "^import template\.core\.base" --type=kt | head -20Repository: openMF/android-client
Length of output: 394
🏁 Script executed:
#!/bin/bash
# Read the KptTheme.kt file to verify the package declaration and exports
echo "=== Content of KptTheme.kt ==="
cat "core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptTheme.kt"Repository: openMF/android-client
Length of output: 2529
Fix incorrect import path for KptTheme — remove .theme segment.
The import should be import template.core.base.designsystem.KptTheme, not import template.core.base.designsystem.theme.KptTheme. The KptTheme function is defined directly in the template.core.base.designsystem package, not in a .theme subpackage. This incorrect import will cause a compilation error.
🤖 Prompt for AI Agents
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt`
at line 52, The import for KptTheme in InterestPage.kt is incorrect; replace the
line importing template.core.base.designsystem.theme.KptTheme with the correct
package import template.core.base.designsystem.KptTheme so the compiler can find
the KptTheme symbol used in InterestPage (look for usages of KptTheme in the
InterestPage composable/function and update the import accordingly).
...Main/kotlin/com/mifos/feature/savings/savingsAccountApproval/SavingsAccountApprovalScreen.kt
Show resolved
Hide resolved
| shape = MaterialTheme.shapes.medium, | ||
| border = BorderStroke( | ||
| 1.dp, | ||
| MaterialTheme.colorScheme.secondaryContainer, | ||
| DesignToken.spacing.dp1, | ||
| KptTheme.colorScheme.outlineVariant, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find KptTheme definition
find . -type f -name "*.kt" | head -20Repository: openMF/android-client
Length of output: 1532
🏁 Script executed:
# Search for KptTheme definition and its properties
rg -n "object KptTheme|class KptTheme" --type=kt -A 30Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Check for shapes property on KptTheme
rg -n "KptTheme\.shapes|val shapes|object shapes" --type=ktRepository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Check for spacing property on KptTheme
rg -n "KptTheme\.spacing|val spacing|object spacing" --type=ktRepository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Get context around lines 292-295 in SearchRecordScreen.kt
cat -n feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt | sed -n '290,300p'Repository: openMF/android-client
Length of output: 508
🏁 Script executed:
# Search for KptTheme definition with correct file type flag
rg -n "object KptTheme|class KptTheme" --type kotlin -A 50Repository: openMF/android-client
Length of output: 12631
🏁 Script executed:
# Search for shapes property usage on KptTheme
rg "KptTheme.*shapes" --type kotlinRepository: openMF/android-client
Length of output: 9147
🏁 Script executed:
# Search for spacing property usage on KptTheme
rg "KptTheme.*spacing" --type kotlinRepository: openMF/android-client
Length of output: 50378
🏁 Script executed:
# Find KptTheme file location
find . -type f -name "*KptTheme*" 2>/dev/nullRepository: openMF/android-client
Length of output: 256
🏁 Script executed:
# Find KptShapes definition
rg "object KptShapes|class KptShapes|interface KptShapes" --type kotlin -A 20Repository: openMF/android-client
Length of output: 8270
🏁 Script executed:
# Find KptSpacing definition
rg "object KptSpacing|class KptSpacing|interface KptSpacing" --type kotlin -A 20Repository: openMF/android-client
Length of output: 10570
🏁 Script executed:
# Check what DesignToken.spacing.dp1 is
rg "DesignToken.*spacing.*dp1|val dp1" --type kotlin -B 2 -A 2Repository: openMF/android-client
Length of output: 15767
Replace MaterialTheme.shapes.medium with KptTheme.shapes.medium.
Line 292 uses MaterialTheme.shapes.medium instead of the equivalent KptTheme.shapes.medium, inconsistent with the rest of the refactor. DesignToken.spacing.dp1 on line 294 is acceptable for border widths, as KptTheme.spacing only provides tokens starting at 4.dp (xs), and DesignToken.spacing.dp1 remains the appropriate choice for precise 1.dp borders throughout the codebase.
🤖 Prompt for AI Agents
In
`@feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt`
around lines 292 - 295, Replace the inconsistent use of
MaterialTheme.shapes.medium with KptTheme.shapes.medium in the composable where
the shape is set (currently using MaterialTheme.shapes.medium in the shape = ...
parameter) so it aligns with the refactor; leave the BorderStroke width as
DesignToken.spacing.dp1 since KptTheme.spacing doesn't provide a 1.dp token.
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
24061e7 to
7c0ebc4
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/groupLoanAccount/GroupLoanAccountScreen.kt (1)
296-301:⚠️ Potential issue | 🟡 MinorPre-existing bug: Disbursement date picker dismiss button clears the wrong flag.
Line 298 sets
showSubmissionDatePicker = falseinstead ofshowDisbursementDatePicker = false. This means dismissing the disbursement date picker won't actually close it (theonDismissRequestat line 282 is the only thing closing it). Not introduced by this PR, but since the file is being touched, worth fixing.🐛 Proposed fix
dismissButton = { TextButton( onClick = { - showSubmissionDatePicker = false + showDisbursementDatePicker = false }, ) { Text(stringResource(Res.string.feature_loan_cancel)) } },feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/addAddress/AddAddressScreen.kt (2)
351-402:⚠️ Potential issue | 🟡 MinorInconsistent minimum heights between Cancel and Submit buttons.
The Cancel button uses
DesignToken.sizes.avatarMedium(line 355) while the Submit button usesDesignToken.sizes.iconExtraLarge(line 383) forheightIn. These two buttons sit side-by-side in the same row, so differing minimum heights will produce a visually unbalanced bottom bar. Additionally, both still referenceDesignToken.sizesrather thanKptTheme— is there aKptThemeequivalent for size tokens?Proposed fix — use the same height token for both buttons
OutlinedButton( modifier = Modifier .fillMaxWidth() .weight(0.4f) - .heightIn(DesignToken.sizes.avatarMedium), + .heightIn(DesignToken.sizes.iconExtraLarge),
534-573:⚠️ Potential issue | 🔴 Critical
validateAddressFieldshas a race condition — validation result is always empty.
scope.launch { … }is asynchronous, so the function returns the initial emptyAddressValidationResult()before the coroutine body assigns the real errors. This meansaddressErrors.isValidin the caller (line 214) is alwaystrue, and no field-level error messages are ever surfaced.This is a pre-existing bug (not introduced by this PR), but it directly breaks form validation — the submit button will fire
createAddresseven when required fields are empty.Consider making
validateAddressFieldsasuspendfunction (called inside a coroutine at the call site) or removing the coroutine entirely sincegetStringfrom Compose Resources can be called in a suspend context or hoisted differently.Simplest fix — drop the coroutine wrapper
-private fun validateAddressFields( +private suspend fun validateAddressFields( addressTypeId: Int, addressLine1: String, city: String, stateProvinceId: Int, countryId: Int, postalCode: String, - scope: CoroutineScope, ): AddressValidationResult { - var result = AddressValidationResult() - scope.launch { - val addressTypeError = - if (addressTypeId <= 0) getString(Res.string.feature_client_address_type_error) else null - ... - result = AddressValidationResult(...) - } - return result + val addressTypeError = + if (addressTypeId <= 0) getString(Res.string.feature_client_address_type_error) else null + val addressLine1Error = + if (addressLine1.isBlank()) getString(Res.string.feature_client_address_line_error) else null + val cityError = + if (city.isBlank()) getString(Res.string.feature_client_address_city_error) else null + val stateProvinceError = + if (stateProvinceId <= 0) getString(Res.string.feature_client_address_state_province_error) else null + val countryError = + if (countryId <= 0) getString(Res.string.feature_client_address_country_error) else null + val postalCodeError = + if (postalCode.isBlank()) getString(Res.string.feature_client_address_postal_code_error) else null + return AddressValidationResult( + addressTypeError = addressTypeError, + addressLine1Error = addressLine1Error, + cityError = cityError, + stateProvinceError = stateProvinceError, + countryError = countryError, + postalCodeError = postalCodeError, + ) }The call site in
onSubmitClick(line 197) would then need to launch a coroutine and call the now-suspendfunction inside it.feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/newIndividualCollectionSheet/NewIndividualCollectionSheetScreen.kt (1)
326-397:⚠️ Potential issue | 🟡 MinorPre-existing:
CollectionSheetDialogContentwraps content in a nestedMifosBottomSheet.This composable creates its own
MifosBottomSheet(line 326), but it's already called from within aMifosBottomSheetat line 184. This results in nested bottom sheets, which is likely unintended and could cause visual/behavioral issues. Not introduced by this PR, but worth addressing.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (1)
571-576:⚠️ Potential issue | 🟡 MinorMissing top padding on
MifosTwoButtonRow, inconsistent withTermsPage.In
TermsPage.kt(line 145), theMifosTwoButtonRowhasmodifier = Modifier.padding(top = KptTheme.spacing.sm), but here it has none. This likely results in the buttons sitting flush against the scrollable content above. Consider adding the same padding for visual consistency across pages.Proposed fix
MifosTwoButtonRow( firstBtnText = stringResource(Res.string.feature_recurring_deposit_back), secondBtnText = stringResource(Res.string.feature_recurring_deposit_next), onFirstBtnClick = { onAction(RecurringAccountAction.OnBackPress) }, onSecondBtnClick = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.OnSettingNext) }, + modifier = Modifier.padding(top = KptTheme.spacing.sm), )feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (1)
305-314:⚠️ Potential issue | 🟠 MajorDuplicate map key silently drops an entry.
Lines 307 and 314 both use
Res.string.feature_recurring_deposit_periodas the map key. In Kotlin'smapOf, the second entry overwrites the first, so the interest period value from line 307 is silently lost. Assign the second key a different name (e.g.,Res.string.feature_recurring_deposit_minimum_balance_for_interest_calculationor similar).feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (1)
556-560:⚠️ Potential issue | 🔴 CriticalBug: Row click toggles the wrong state field.
The
clickablemodifier on this row uses!state.isCheckedEqualAmortizationinstead of!state.isCheckedInterestPartialPeriod. This means clicking the "Calculate interest for partial period" row actually toggles the equal amortization flag instead of the interest partial period flag. TheCheckboxon line 564 correctly usesisCheckedInterestPartialPeriod, so the clickable and checkbox are out of sync.🐛 Proposed fix
Row( Modifier.fillMaxWidth() .clickable { - onAction(NewLoanAccountAction.OnInterestPartialPeriodCheckChange(!state.isCheckedEqualAmortization)) + onAction(NewLoanAccountAction.OnInterestPartialPeriodCheckChange(!state.isCheckedInterestPartialPeriod)) },feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/syncSurvey/SyncSurveysDialog.kt (1)
441-451:⚠️ Potential issue | 🟡 MinorPreview is not wrapped in the
KptThemeprovider.Since the dialog content now reads
KptTheme.colorScheme,KptTheme.spacing, andKptTheme.shapes, the preview will likely crash if these rely onCompositionLocalvalues that aren't provided. Wrap the preview content inKptTheme { ... }.🛠️ Proposed fix
private fun SyncSurveysDialogPreview() { - Column { - SyncSurveysDialog(uiState = SyncSurveysDialogUiState.DismissDialog, closeDialog = {}) - ... + KptTheme { + Column { + SyncSurveysDialog(uiState = SyncSurveysDialogUiState.DismissDialog, closeDialog = {}) + ... + } } }feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupDetails/GroupDetailsScreen.kt (1)
494-514:⚠️ Potential issue | 🟡 MinorPre-existing bug: "active and in arrears" branch is unreachable.
The
whenbranch at Line 507 (status?.active == true && inArrears == true) can never be reached becausestatus?.active == trueis already matched by the first branch at Line 495. The arrears case should be checked before the plain active case.🐛 Proposed fix — reorder branches
color = when { + loanAccount.status?.active == true && loanAccount.inArrears == true -> { + Color.Red + } + loanAccount.status?.active == true -> { Color.Green } loanAccount.status?.waitingForDisbursal == true -> { Color.Blue } loanAccount.status?.pendingApproval == true -> { Color.Yellow } - loanAccount.status?.active == true && loanAccount.inArrears == true -> { - Color.Red - } - else -> { Color.DarkGray }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt (1)
199-209:⚠️ Potential issue | 🟡 MinorUnreachable branch:
Feature.VIEW_DOCUMENTcheck is dead code.Line 202 tests
state.feature == Feature.VIEW_DOCUMENT, but this entire block is nested insideif (state.feature != Feature.VIEW_DOCUMENT)(line 190). Thewhenbranch on line 202 can never be true.Proposed fix
Text( text = when { - state.feature == Feature.VIEW_DOCUMENT -> stringResource(Res.string.client_identifier_title) - state.documentKey == null -> stringResource(Res.string.client_update_document_title) - else -> stringResource(Res.string.add_document_title) }, style = KptTheme.typography.titleMedium, )feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/components/ProfileCard.kt (1)
149-162:⚠️ Potential issue | 🟠 MajorUse
KptMaterialThemeinstead ofMifosThemein the preview.The preview wraps
ProfileCardinMifosTheme, butProfileCardreads theme values fromKptThemecomposition locals (colorScheme, typography, spacing, shapes).MifosThemedoes not provide these composition locals—it only provides separate design token composition locals. UseKptMaterialThemeinstead, which provides both Material3 theme compatibility and the required KPT composition locals.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt (1)
430-442:⚠️ Potential issue | 🟡 Minor
Color.Greenis still hardcoded, inconsistent with the theme migration.Withdrawals correctly use
KptTheme.colorScheme.error(line 436) and the default branch usesKptTheme.colorScheme.onSurface(line 440), but deposits still use a rawColor.Green. This will look wrong on dark themes and breaks the consistency of this migration. Consider using a semantic color fromKptTheme.colorScheme(e.g., a success/tertiary token, or define one if the palette lacks it).Proposed fix
color = when { transaction.transactionType?.deposit == true -> { - Color.Green + KptTheme.colorScheme.tertiary // or a dedicated success color token }feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/syncSavingsAccountTransaction/SyncSavingsAccountTransactionScreen.kt (1)
122-129:⚠️ Potential issue | 🟠 MajorPre-existing bug:
syncSavingsAccountTransactionsis referenced but never invoked.On line 124, the function is referenced as a value (
syncSavingsAccountTransactions) but never called. It needs()to actually trigger the sync.🐛 Proposed fix
- true -> syncSavingsAccountTransactions + true -> syncSavingsAccountTransactions()feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (1)
170-201:⚠️ Potential issue | 🟡 MinorHard-coded
"Not Available"strings break localization.Lines 171, 173, 175, 180, 192, and 201 use raw
"Not Available"string literals instead ofstringResource(...). This is an i18n regression — these user-facing strings should be extracted to resource files so they can be translated.Example fix for one occurrence
- loan.accountNo ?: "Not Available" + loan.accountNo ?: stringResource(Res.string.feature_client_not_available)Apply the same pattern to all six occurrences in this block.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/LoanChargeDialogScreen.kt (1)
319-330:⚠️ Potential issue | 🟡 MinorPre-existing: side-effect during composition in the success branch.
This isn't introduced by this PR, but worth noting:
onSuccess()(line 329) is called directly during composition without being wrapped inLaunchedEffect. This means it fires on every recomposition of this branch, not just once. Similarly,scope.launch { snackbarHostState.showSnackbar(...) }(line 324) launches a new coroutine on each recomposition. Additionally, thesnackbarHostStatecreated at line 113 is never wired to aSnackbarHost, so the snackbar message is silently discarded.Consider wrapping both in a
LaunchedEffect(Unit)in a follow-up.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
840-855:⚠️ Potential issue | 🟠 MajorPreview will crash—wrap content in
KptThemeinstead ofMifosTheme.The composable uses
KptThemethroughout (40+ references toKptTheme.colorScheme,KptTheme.spacing,KptTheme.typography,KptTheme.shapes), but the preview at line 840 wraps it only inMifosTheme. SinceMifosThemedoesn't provideKptTheme's CompositionLocals, the preview will fail or use incorrect defaults.Change line 840 to:
KptTheme {Or, if
MifosThemeis intentionally needed, nestKptThemeinside it:MifosTheme { KptTheme {
🤖 Fix all issues with AI agents
In
`@feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInbox/CheckerInboxScreen.kt`:
- Line 88: The import for KptTheme is incorrect; update the import statement in
CheckerInboxScreen.kt to reference the actual package where KptTheme is declared
by replacing import template.core.base.designsystem.theme.KptTheme with import
template.core.base.designsystem.KptTheme so the KptTheme symbol resolves
correctly.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt`:
- Around line 290-291: The code mixes DesignToken and KptTheme spacing systems
(e.g., DesignToken.spacing.dp44 vs KptTheme.spacing.md, and
DesignToken.spacing.largeIncreased) and also uses a hardcoded 18.dp; standardize
by either adding the missing tokens to KptTheme (add dp44 and largeIncreased to
KptTheme.spacing and add dp18 to KptTheme.sizes) or replace usages with the
closest KptTheme tokens (e.g., map dp44 → KptTheme.spacing.xxl or another agreed
token, map largeIncreased → KptTheme.spacing.xxl/xl as appropriate) and replace
hardcoded 18.dp with DesignToken.sizes.dp18 or KptTheme.sizes.dp18; update the
references DesignToken.spacing.dp44, DesignToken.spacing.largeIncreased,
KptTheme.spacing.md, and the hardcoded 18.dp so the file consistently uses the
same spacing/sizes system.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt`:
- Line 60: KptTheme.kt currently declares package
template.core.base.designsystem but all consumers (e.g., imports in
KptMaterialTheme.kt and other files) expect
template.core.base.designsystem.theme; move KptTheme.kt into the theme
subdirectory and change its package declaration to
template.core.base.designsystem.theme (or alternatively update all imports to
remove .theme, but prefer moving the file to keep the existing import pattern
consistent).
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt`:
- Around line 129-130: In ClientSignatureScreen replace the incorrect spacing
token used for icon dimensions: change the Modifier.size(...) call that
currently uses KptTheme.spacing.lg to use DesignToken.sizes.iconMedium instead
(match the existing pattern used where tint = KptTheme.colorScheme.primary and
modifier = Modifier.size(DesignToken.sizes.iconMedium)); update the single
Modifier.size invocation to use DesignToken.sizes.iconMedium so the icon uses
the semantic size token rather than a spacing token.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientStaff/ClientStaffScreen.kt`:
- Line 141: In ClientStaffScreen.kt replace the zero-size Spacer using
Modifier.padding(KptTheme.spacing.sm) (used inside the Row) with an explicit
horizontal spacer using Modifier.width(KptTheme.spacing.sm) so the spacing is
predictable and semantic; update the Spacer invocation in the Row where it
currently uses Modifier.padding to use a width modifier instead.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/ChargesPage.kt`:
- Line 37: The file still uses DesignToken.sizes.iconSmall (and imports
DesignToken) while the theme was migrated to KptTheme; replace the
DesignToken.sizes.iconSmall usage in ChargesPage (wherever referenced) with
KptTheme.spacing.md (or add a sizes token onto KptTheme if you prefer to
preserve a dedicated icon size) and remove the now-unused import of
com.mifos.core.designsystem.theme.DesignToken; update any references to ensure
they use KptTheme (colorScheme/typography/shapes/spacing/elevation)
consistently.
In
`@feature/document/src/commonMain/kotlin/com/mifos/feature/document/documentDialog/DocumentDialogScreen.kt`:
- Around line 344-347: The Text composable in DocumentDialogScreen contains a
hardcoded user-facing string; extract "Supported formats:
xls,xlsx,pdf,doc,docx,png,jpeg,jpg." into a string resource (e.g.,
supported_formats) and replace the literal with a lookup (e.g.,
stringResource(R.string.supported_formats)) in the Text call inside
DocumentDialogScreen so the UI uses the resource for consistency and
localization.
- Around line 322-326: The trailing error icon for the selected file
OutlinedTextField is currently conditioned on descriptionError instead of the
field's isError state; update the trailingIcon lambda in the selected file
OutlinedTextField (the trailingIcon block that currently checks
descriptionError) to check fileError instead so the icon matches the isError
binding (use the existing MifosIcons.Error icon and null contentDescription).
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/LoanChargeDialogScreen.kt`:
- Around line 185-205: The close Icon in LoanChargeDialogScreen (inside the
Column/Row -> IconButton -> Icon using MifosIcons.Close) is sized with
KptTheme.spacing.xl; replace those width/height uses with the semantically
correct design token DesignToken.sizes.iconLarge (or iconExtraLarge if you
prefer 36.dp) so the icon uses the dedicated size tokens instead of spacing;
optionally, if you want a consistent theme API, expose sizes via KptTheme (e.g.,
KptTheme.sizes.iconLarge) and use that in the Icon modifier.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt`:
- Line 95: Fix the checkbox toggle and icon size: in the clickable row handler
that currently negates state.isCheckedEqualAmortization, change it to toggle
state.isCheckedInterestPartialPeriod so the row toggles the same boolean the
Checkbox reads (reference state.isCheckedInterestPartialPeriod and the clickable
lambda in TermsPage). Also replace the icon size usage of KptTheme.spacing.md
with a proper size token (restore or use DesignToken.sizes.iconSmall or an
equivalent icon size token) where the Icon is created so spacing tokens are not
used for icon sizing.
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt`:
- Around line 89-95: The section header Text in PreviewPage (the Text composable
using KptTheme.typography.labelLarge) must be changed to use
KptTheme.typography.titleMedium to match the sibling pages (TermsPage and
SettingsPage) so step headers are consistent; update the style reference in
PreviewPage.kt from labelLarge to titleMedium and keep the existing color
(KptTheme.colorScheme.onSurface) unchanged.
In
`@feature/report/src/commonMain/kotlin/com/mifos/feature/report/reportDetail/ReportDetailScreen.kt`:
- Around line 491-573: The onOptionSelected handlers for MifosTextFieldDropdown
using fundList, currencyList, parCalculatorList, savingsAccountDepositList,
glAccountList, and obligationDateList use unsafe indexing (e.g.,
fundList[index].row.first()) which can throw; update each handler (the
onOptionSelected lambdas in the MifosTextFieldDropdown for selectedFund,
selectedCurrency, selectedParCalculator, selectedSavingsAccountDeposit,
selectedGlAccount, selectedObligationDate) to use safe-access like
getOrNull(index)?.row?.getOrNull(0) ?: "" so you consistently mirror the safe
pattern used elsewhere (officeList, loanPurpose, loanOfficer, products).
In
`@feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt`:
- Around line 517-521: The border width is using a spacing token
(KptTheme.spacing.xs) which is too large for strokes; in the
SavingsAccountSummaryScreen composable replace the width argument on the .border
call (currently using KptTheme.spacing.xs) with a small fixed Dp (e.g., 1.dp) or
a dedicated border token (for example KptTheme.border.small) and ensure the
appropriate import for dp is present; update the .border invocation in
SavingsAccountSummaryScreen to use that small stroke value instead of the
spacing token.
🧹 Nitpick comments (43)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (1)
105-140: Pre-existing:LazyColumnwith a singleitem {}wrappingforEachIndexeddefeats lazy rendering.This isn't introduced by your PR, but worth noting: placing
forEachIndexedinside oneitem { }block means all accounts are composed eagerly within that single item, negatingLazyColumn's virtualization benefit. UsingitemsIndexed(state.accounts)would allow proper item recycling.♻️ Suggested refactor
- LazyColumn { - item { - state.accounts.forEachIndexed { index, account -> - MifosActionsShareListingComponent( - ... - ) - Spacer(Modifier.height(KptTheme.spacing.sm)) - } - } - } + LazyColumn { + itemsIndexed(state.accounts) { index, account -> + MifosActionsShareListingComponent( + ... + ) + Spacer(Modifier.height(KptTheme.spacing.sm)) + } + }feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountApproval/SavingsAccountApprovalScreen.kt (1)
191-241: Theme token migration looks consistent overall.The replacements from
MaterialThemetoKptThemefor typography, color, and spacing are clean and align with the broader PR pattern.Minor observation: line 229 uses
DesignToken.spacing.dp44while all other spacing references useKptTheme.spacing.*. This mixing is understandable ifKptThemedoesn't expose a 44dp token, but consider whether a semantic alias (e.g.,KptTheme.spacing.buttonMinHeightor similar) would be clearer for maintainability.feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/generateCollectionSheet/GenerateCollectionSheetScreen.kt (1)
303-303:heightInsets a minimum height, not a fixed height — preferheightfor spacers.
Modifier.heightIn(KptTheme.spacing.md)passes the value as theminparameter, leavingmaxunconstrained. For spacers intended as fixed-size gaps,Modifier.height(...)is the idiomatic choice. The same applies to all otherSpacerusages on lines 321, 330, 349, 374, 391, and 408.In practice this won't cause layout issues here (no weighted children in the
Column), but it's semantically misleading.Suggested fix (apply the same pattern to all spacers)
- Spacer(modifier = Modifier.heightIn(KptTheme.spacing.md)) + Spacer(modifier = Modifier.height(KptTheme.spacing.md))feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/groupLoanAccount/GroupLoanAccountScreen.kt (1)
457-458: Consider migratingDesignToken.spacing.dp164to a semantic token.
DesignToken.spacing.dp164is used for the dropdown width in two places. UnlikeKptTheme.spacing.md/smwhich are semantic,dp164is a magic-number-style token. If the design system supports a semantic alternative (e.g.,KptTheme.sizes.dropdownWidth), prefer that for consistency with the rest of the migration.Also applies to: 490-491
feature/client/src/commonMain/kotlin/com/mifos/feature/client/documentPreviewScreen/DocumentPreviewScreen.kt (1)
218-218: Inconsistent border width: hardcoded1.dpvsDesignToken.spacing.dp1.Lines 115 and 143 use
DesignToken.spacing.dp1for the same border-width purpose. This line should be consistent.Proposed fix
- borderStroke = BorderStroke(1.dp, KptTheme.colorScheme.secondaryContainer), + borderStroke = BorderStroke(DesignToken.spacing.dp1, KptTheme.colorScheme.secondaryContainer),feature/data-table/src/commonMain/kotlin/com/mifos/feature/dataTable/dataTableRowDialog/DataTableRowDialogScreen.kt (1)
138-139: Mixed spacing token sources (DesignToken.spacingvsKptTheme.spacing).Fixed dimensions like icon size (
dp30) and button height (dp50) useDesignToken.spacing, while semantic paddings useKptTheme.spacing. If both token systems expose the same values, consider consolidating to one source for consistency. IfDesignTokenis reserved for raw/fixed dimensions andKptThemefor semantic tokens, a brief comment or convention doc would help future contributors.Also applies to: 173-173
feature/data-table/src/commonMain/kotlin/com/mifos/feature/dataTable/dataTableData/DataTableDataScreen.kt (1)
352-353: Remove leftover mapping comment.The
// TYPOGRAPHY: Mapped to KptTheme titleMediumcomment appears to be a migration note that shouldn't remain in production code.Suggested fix
- // TYPOGRAPHY: Mapped to KptTheme titleMedium - style = KptTheme.typography.titleMedium, + style = KptTheme.typography.titleMedium,feature/data-table/src/commonMain/kotlin/com/mifos/feature/dataTable/dataTable/DataTableScreen.kt (1)
155-159: Nit: simplify identical horizontal/vertical padding.Since both axes use the same token, you can use the single-value overload.
Suggested simplification
.padding( - horizontal = KptTheme.spacing.xs, - vertical = KptTheme.spacing.xs, - ), + KptTheme.spacing.xs, + ),feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/ChargesPage.kt (1)
97-97: Nit: prefer== trueover?: falsefor nullable Boolean.
state.loanTemplate?.overdueCharges?.isNotEmpty() == trueis the more idiomatic Kotlin pattern for checking a nullableBoolean.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanApproval/LoanAccountApprovalScreen.kt (1)
325-331: Remove commented-out code.This block of dead Toast logic has been carried along. It references Android-specific
ToastandR.stringwhich don't belong in acommonMainKMP source set. Remove it to keep the file clean.🧹 Proposed cleanup
-// } else { -// Toast.makeText( -// context, -// context.resources.getString(R.string.feature_loan_error_not_connected_internet), -// Toast.LENGTH_SHORT, -// ).show() -// }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddDocuments/ClientAddDocumentsScreen.kt (1)
208-208: Mixed theme systems:KptThemeandDesignTokenused side-by-side.
DesignToken.sizesis still used on lines 208, 214, 255, and 262, while everything else has been migrated toKptTheme. If the plan is to eventually consolidate all tokens underKptTheme, consider tracking this as a follow-up task to avoid forgetting these remnants.feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt (1)
85-85: Verify that@OptIn(ExperimentalMaterial3Api::class)is actually needed here.None of the APIs directly called in
OfflineDashboardScreenappear to be experimental (e.g.,OutlinedCard,Text,LazyColumn). IfMifosScaffoldpropagates the experimental requirement, this is fine — otherwise the annotation is unnecessary noise.#!/bin/bash # Check if MifosScaffold requires ExperimentalMaterial3Api opt-in ast-grep --pattern '@ExperimentalMaterial3Api $$$ fun MifosScaffold($$$) { $$$ }'feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt (3)
58-62: MixingDesignTokenandKptThemefor spacing tokens.
DesignToken.spacing.dp1is used here while all other spacing references useKptTheme.spacing.*. IfKptThemeexposes an equivalent 1dp spacing token, prefer using it consistently. This would also allow removing theDesignTokenimport entirely.
77-92: Redundant.clip()—OutlinedCardalready clips to itsshape.Line 81 applies
.clip(KptTheme.shapes.small)on the modifier, and line 92 setsshape = KptTheme.shapes.smallon theOutlinedCard. The card component handles clipping internally via its shape parameter, so the explicit.clip()is unnecessary.Proposed fix
modifier = modifier .testTag(it) .padding(KptTheme.spacing.sm) .fillMaxWidth() - .clip(KptTheme.shapes.small) .combinedClickable(
132-133: Remove migration artifact comment.
// SPACING: Replaced 4.dp with KptTheme.spacing.xsreads like a commit note rather than a code comment. It doesn't add value for future readers.Proposed fix
verticalAlignment = Alignment.CenterVertically, - // SPACING: Replaced 4.dp with KptTheme.spacing.xs horizontalArrangement = Arrangement.spacedBy(KptTheme.spacing.xs),feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
43-56: Remove migration-tracking comments.The inline comments like
// PADDING: Replaced DesignToken.padding.large with KptTheme.spacing.lgand// TYPOGRAPHY: Mapped to KptThemeare useful during the migration but add noise post-merge. Consider removing them before merging — the git history already captures what changed.🧹 Remove inline migration comments
- // PADDING: Replaced DesignToken.padding.large with KptTheme.spacing.lg Column(Modifier.fillMaxSize().padding(bottom = KptTheme.spacing.lg)) { Column( modifier = modifier.weight(1f).verticalScroll(rememberScrollState()), ) { Text( text = stringResource(Res.string.feature_recurring_deposit_step_terms), - // TYPOGRAPHY: Mapped to KptTheme style = KptTheme.typography.titleMedium, - // COLOR: Added for theme consistency color = KptTheme.colorScheme.onSurface, ) - // SPACING: Replaced DesignToken.padding.large with KptTheme.spacing.lg Spacer(Modifier.height(KptTheme.spacing.lg))Apply the same cleanup throughout all migrated files.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (2)
715-716: Semantic mismatch: using a spacing token for icon size.
KptTheme.spacing.mdis a spacing/padding token, not a size token. The original code usedDesignToken.sizes.iconSmall, which is semantically correct. If KptTheme doesn't yet expose a dedicated icon size token, consider adding one or using a constant (16.dp/20.dp) rather than overloading spacing semantics.
180-181: Remove migration-tracking inline comments before merging.Every token replacement has a
// SPACING: Replaced ...or// TYPOGRAPHY: Mapped to KptThemecomment (over 30 instances across the file). These are useful during development but add significant noise to the final code and will quickly become stale. Consider removing them before merge — the git history already tracks what changed.Also applies to: 187-189, 192-193, 207-208, 212-214, 217-218
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/ClientAddressScreen.kt (1)
230-234: Use1.dpliteral instead ofDesignToken.spacing.dp1.The file still imports
DesignTokenbut usesKptThemeelsewhere. SinceKptTheme.spacingonly provides larger tokens (xs=4.dp, sm=8.dp, md=16.dp, etc.), the 1.dp border width has no equivalent in the new theme system. Use a simple1.dpliteral for clarity.Suggested fix
-import com.mifos.core.designsystem.theme.DesignToken ... border = BorderStroke( - width = DesignToken.spacing.dp1, + width = 1.dp, color = KptTheme.colorScheme.outlineVariant, ),feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/syncSurvey/SyncSurveysDialog.kt (1)
246-389: Reduce repetitivecolor = KptTheme.colorScheme.onSurfaceon every Text.The same color is applied to ~16 Text composables individually. You could set it once at the Column level using
CompositionLocalProvider(LocalContentColor provides KptTheme.colorScheme.onSurface), and Text will inherit the color automatically.♻️ Suggested approach
+ CompositionLocalProvider(LocalContentColor provides KptTheme.colorScheme.onSurface) { Column( modifier = Modifier .fillMaxWidth() .padding(KptTheme.spacing.sm), ) { // ... all Rows with Text - remove `color = KptTheme.colorScheme.onSurface` from each } + }feature/document/src/commonMain/kotlin/com/mifos/feature/document/documentDialog/DocumentDialogScreen.kt (1)
277-278: Inconsistent token usage:DesignToken.spacingvsKptTheme.spacing.These two lines use
DesignToken.spacing.dp30while every other spacing value in this file usesKptTheme.spacing.*. IfKptThemedoesn't expose adp30equivalent, consider adding one to keep a single source of truth; otherwise, switch to theKptThemetoken for consistency.feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt (1)
258-261: Nit: Mixed token systems inBorderStroke.
widthusesDesignToken.spacing.dp1whilecolorusesKptTheme.colorScheme.outlineVariant. IfKptThemedoesn't expose adp1equivalent yet, this is fine for now — just flagging for future cleanup when the migration is complete.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (2)
227-231: HardcodedColor.Redheader background is inconsistent with the theme migration.The header background on line 229 still uses
Color.Red.copy(alpha = .5f)while the text inside it usesKptTheme.colorScheme.onErrorContainer. Consider replacing this withKptTheme.colorScheme.errorContainer(or the appropriate semantic color) for consistency with the design-system migration.♻️ Suggested fix
Box( modifier = Modifier - .background(Color.Red.copy(alpha = .5f)) + .background(KptTheme.colorScheme.errorContainer) .fillMaxWidth(), ) {
136-146: Status indicator colors are still hardcoded.
Color.Green,Color.Red, andColor.Blueare used for status indicators. While these are semantic (complete/overdue/pending), they won't adapt to dark mode or theme changes. Consider defining these as named colors in the design system if they aren't already.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/component/RepaymentPeriodCard.kt (1)
65-67: Mixed usage ofDesignToken.spacingandKptTheme.spacing.
DesignToken.spacing.dp1is used for the border width (also on line 73), while all other spacing references have been migrated toKptTheme.spacing. IfKptThemeexposes an equivalent token, consider using it for consistency. If not, this is fine as-is.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientStaff/ClientStaffScreen.kt (1)
128-129:DesignToken.sizes.iconAveragenot migrated toKptTheme.Two occurrences of
DesignToken.sizesremain while all other tokens (spacing, typography, colorScheme) have been migrated toKptTheme. IfKptThemedoesn't expose size tokens yet, this is fine to defer — just flagging for consistency tracking.Also applies to: 150-151
feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupDetails/GroupDetailsScreen.kt (2)
594-596: Inconsistent icon sizing — hardcoded24.dpnot migrated to theme token.In
MifosLoanAccountExpendableCard(Line 432), the equivalentIconButtonsize usesDesignToken.sizes.iconMedium, but here inMifosSavingsAccountExpendableCardit's still hardcoded to24.dp. This should use the same token for consistency with the migration.♻️ Proposed fix
IconButton( modifier = Modifier - .size(24.dp), + .size(DesignToken.sizes.iconMedium), onClick = { expendableState = !expendableState },
468-471: Fragile fixed-heightLazyColumnwith inconsistent item height assumptions.Line 470 assumes 52dp per loan item, while Line 633 assumes 50dp per savings item, despite the rows having nearly identical layouts. These magic numbers are fragile — if text size or padding changes (as this migration does), the heights may clip or leave gaps. Consider using
Modifier.heightIn(max = ...)or wrapping in a scrollable container instead.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt (1)
459-465: Preferstyle = KptTheme.typography.labelLargeover extracting individual properties.Manually extracting
fontSize,letterSpacing(and sometimeslineHeight) from aTextStyleis verbose and error-prone. Notice thatlineHeightis included in the bottom barTextcomposables (Lines 882, 906) but omitted here, creating an inconsistency. TheTextcomposable accepts astyleparameter directly:♻️ Suggested change (apply similarly to Lines 505-511, 590-596)
Text( text = stringResource(Res.string.feature_client_personal_details), fontWeight = FontWeight.SemiBold, - fontSize = KptTheme.typography.labelLarge.fontSize, - letterSpacing = KptTheme.typography.labelLarge.letterSpacing, - color = KptTheme.colorScheme.onSurface, + style = KptTheme.typography.labelLarge, + color = KptTheme.colorScheme.onSurface, )feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (1)
47-47: DesignToken.sizes.iconSmall requires KptTheme to expose size tokens first.Line 90 uses
DesignToken.sizes.iconSmall(16.dp), making DesignToken the only non-KptTheme design token in this file. However, KptTheme currently only exposescolorScheme,typography,shapes,spacing, andelevation—it has no size tokens. To migrate away from DesignToken entirely, size-related tokens would first need to be added to KptTheme.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountTransaction/SavingsAccountTransactionScreen.kt (2)
426-437: Inconsistent use ofDesignToken.spacing.dp46alongsideKptTheme.spacing.*.The PR aims to centralize on
KptTheme, yet the button height still usesDesignToken.spacing.dp46while all other spacings referenceKptTheme.spacing. Consider exposingdp46throughKptTheme.spacingas well for consistency, or at least leave a comment explaining whyDesignTokenis used here.
478-498:FarApartTextItemis duplicated across multiple feature modules.This composable is identical to the one in
SavingsAccountSummaryScreen.ktand very similar toLoanRepaymentScreen.kt. Consider extracting it into a shared UI component (e.g., incore:ui) to reduce duplication as the theme migration progresses.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt (1)
247-247: Semantic mismatch: spacing token used for icon size.
KptTheme.spacing.lgis a spacing/layout token being used as an icon dimension (Modifier.size(...)). If the spacing value changes for layout reasons, it would unintentionally resize this icon. Prefer a dedicated size token (e.g.,DesignToken.sizes.*or a futureKptTheme.sizes.*) to decouple icon dimensions from spacing.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt (1)
275-277: No-op.map { it }— can be removed.
state.statusList.map { it }returns an identical list. If the intent is to convert to aListfrom another collection type, use.toList()instead; otherwise just passstate.statusListdirectly.Proposed fix
- options = state.statusList.map { - it - }, + options = state.statusList,feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (1)
391-391: Spacing token used for icon sizing is semantically incorrect.
KptTheme.spacing.lgis a spacing/padding token, not a component-size token. Using it forModifier.size(...)conflates two distinct design-system concepts. Consider using a dedicated size token (e.g., fromDesignToken.sizes) or introducing an icon-size token in KptTheme to keep the semantic intent clear.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/components/ProfileCard.kt (1)
107-111: Spacing token used for icon size — prefer a size token for semantic clarity.Line 110 uses
KptTheme.spacing.mdto set the chevron icon's size, while line 136 correctly usesDesignToken.sizes.iconSmallfor the group icon. Icon dimensions should reference size tokens (e.g.,DesignToken.sizes.iconSmallor a similar KptTheme size token) rather than spacing tokens, even if the numeric value happens to match today.♻️ Suggested fix
Icon( imageVector = MifosIcons.ChevronRight, contentDescription = null, - modifier = Modifier.size(KptTheme.spacing.md), + modifier = Modifier.size(DesignToken.sizes.iconSmall), tint = KptTheme.colorScheme.onPrimary.copy(alpha = 0.6f), )feature/center/src/commonMain/kotlin/com/mifos/feature/center/centerList/ui/CenterListScreen.kt (1)
282-306: Mixed token sources:DesignToken.sizesvsKptTheme.spacing.Sizes use
DesignToken.sizes.*(lines 282, 306) while spacing usesKptTheme.spacing.*(line 302). IfKptThemeis intended to be the single source of truth for design tokens going forward, consider whether sizes should also be accessed throughKptThemefor consistency — or document why the two token sources coexist.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt (2)
795-795: Another leftoverDesignToken.spacingreference in the dialog.
DesignToken.spacing.largeIncreasedshould be replaced with the equivalentKptTheme.spacingtoken for consistency with the rest of this file.
843-844: Hardcoded18.dpfor icon size — consider using a design token.Other icon sizes in this file use
DesignToken.sizes.*. This one should also use a token for consistency.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (1)
83-83: Incomplete migration:DesignTokenis still used alongsideKptTheme.The file imports and uses both
DesignToken(forsizes.inputHeight,sizes.avatarLarge,sizes.buttonHeight) andKptTheme(for spacing, typography, colors). If the goal is to fully centralize on KptTheme, these remainingDesignTokenusages should be migrated too. IfDesignToken.sizesis intentionally kept separate fromKptTheme, this is fine — but worth confirming to avoid a half-migrated state.Also applies to: 339-342, 353-356, 368-371, 383-386, 398-401, 415-418, 595-595, 621-621
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)
665-668: Semantic mismatch:DesignToken.spacing.dp46used as a button height.
DesignToken.spacing.dp46is a spacing token, but it's being used asheightInfor a button. This should probably reference a size token (e.g.,DesignToken.sizes.buttonHeight) for semantic clarity. This may be a pre-existing issue, but worth noting during the migration.feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt (1)
59-85: Remove commented-out code and reconsider duplicating MifosScaffold's built-in navigation.Lines 60–61 contain commented-out
title/onBackPressedparams that should be deleted. Additionally, replacing MifosScaffold's built-in title/back-navigation with a hand-rolledTopAppBaris inconsistent with other screens in this PR (e.g.,LoanRepaymentScreenat line 154 still usestitleandonBackPressed). If the goal is to style the TopAppBar with KptTheme colors, consider updatingMifosScaffolditself to accept theme-driven colors rather than duplicating the pattern per-screen.Minimum fix: remove dead comments
MifosScaffold( -// title = stringResource(Res.string.feature_about), -// onBackPressed = onBackPressed, snackbarHostState = remember { SnackbarHostState() }, topBar = {feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/LoanChargeDialogScreen.kt (1)
293-293: Dual token source confirmed as codebase convention — but ensure non-standard dimensions can't be expressed semantically.This mixing pattern is present in 39 files across the codebase and is intentional:
KptTheme.spacingprovides semantic tokens for standard sizes (.sm,.md,.lg,.xl), whileDesignToken.spacingprovides exact pixel values for non-standard dimensions. However, clarify whydp52anddp300cannot be expressed as named tokens inKptTheme.spacing— if they recur across multiple features, consider adding semantic equivalents to avoid fragmentation.
| import org.jetbrains.compose.ui.tooling.preview.PreviewParameter | ||
| import org.jetbrains.compose.ui.tooling.preview.PreviewParameterProvider | ||
| import org.koin.compose.viewmodel.koinViewModel | ||
| import template.core.base.designsystem.theme.KptTheme |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Search for KptTheme file ==="
find . -type f -name "*KptTheme*" 2>/dev/null | grep -E "\.(kt|java)$"
echo ""
echo "=== Search for KptTheme class/object declaration ==="
rg "class KptTheme|object KptTheme" --type=kt -B2 | head -30
echo ""
echo "=== All imports of KptTheme in the codebase ==="
rg "import.*KptTheme" --type=kt
echo ""
echo "=== Search for template.core.base package in codebase ==="
rg "template\.core\.base" --type=kt
echo ""
echo "=== DesignToken location for comparison ==="
find . -type f -name "*DesignToken*" 2>/dev/null | grep -E "\.(kt|java)$"Repository: openMF/android-client
Length of output: 685
🏁 Script executed:
#!/bin/bash
echo "=== KptTheme.kt file content (first 50 lines) ==="
cat -n "./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptTheme.kt" | head -50
echo ""
echo "=== Check for theme subpackage/directory ==="
find "./core-base/designsystem" -type d -name "theme"
echo ""
echo "=== All files in template/core/base/designsystem directory ==="
ls -la "./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/" 2>/dev/null | head -20Repository: openMF/android-client
Length of output: 3151
🏁 Script executed:
#!/bin/bash
echo "=== CheckerInboxScreen.kt lines 80-95 ==="
sed -n '80,95p' "./feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInbox/CheckerInboxScreen.kt" | cat -nRepository: openMF/android-client
Length of output: 925
Incorrect import path for KptTheme.
The import at line 88 is import template.core.base.designsystem.theme.KptTheme, but KptTheme.kt is located in the template.core.base.designsystem package, not a theme subpackage. The correct import should be import template.core.base.designsystem.KptTheme.
🤖 Prompt for AI Agents
In
`@feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInbox/CheckerInboxScreen.kt`
at line 88, The import for KptTheme is incorrect; update the import statement in
CheckerInboxScreen.kt to reference the actual package where KptTheme is declared
by replacing import template.core.base.designsystem.theme.KptTheme with import
template.core.base.designsystem.KptTheme so the KptTheme symbol resolves
correctly.
| .heightIn(DesignToken.spacing.dp44) | ||
| .padding(horizontal = KptTheme.spacing.md), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine KptTheme to see what spacing tokens are available
fd "KptTheme" -t f -e kt --exec head -100 {} \; 2>/dev/null | head -50
echo "=== Searching for KptTheme.spacing definition ==="
rg "object KptTheme|val spacing" -A 20 -B 2 --type=kt | head -80
echo "=== Searching for all DesignToken.spacing usage ==="
rg "DesignToken\.spacing" --type=kt -n | head -40Repository: openMF/android-client
Length of output: 2227
🏁 Script executed:
#!/bin/bash
# Look at the specific file and lines mentioned
echo "=== Examining ClientDetailsScreen.kt around line 290-291 ==="
sed -n '285,295p' feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt
echo "=== Examining line 795 ==="
sed -n '790,800p' feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt
echo "=== Examining line 844 ==="
sed -n '840,850p' feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.ktRepository: openMF/android-client
Length of output: 1577
🏁 Script executed:
#!/bin/bash
# Count total usages of DesignToken.spacing vs KptTheme.spacing in ClientDetailsScreen
echo "=== DesignToken.spacing usage count in ClientDetailsScreen ==="
rg "DesignToken\.spacing" feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt --type=kt | wc -l
echo "=== KptTheme.spacing usage count in ClientDetailsScreen ==="
rg "KptTheme\.spacing" feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt --type=kt | wc -l
echo "=== Full list of DesignToken.spacing in ClientDetailsScreen ==="
rg "DesignToken\.spacing" feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt --type=kt -nRepository: openMF/android-client
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Find KptTheme definition and check what spacing tokens it offers
fd "KptTheme" --type=f -e kt | xargs rg "object KptTheme|spacing\s*=" --type=kt -B 3 -A 30 | head -150Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find KptTheme and DesignToken files directly
echo "=== Finding KptTheme files ==="
find . -name "*KptTheme*" -type f
echo "=== Finding DesignToken files ==="
find . -name "*DesignToken*" -type f
echo "=== Finding spacing-related files ==="
find . -path "*/designsystem/*" -name "*[Ss]pacing*" -type fRepository: openMF/android-client
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Search for KptTheme.spacing definition using grep
echo "=== KptTheme.spacing definition ==="
grep -r "object KptTheme\|val spacing" --include="*.kt" | grep -i spacing | head -30
echo "=== DesignToken.spacing definition ==="
grep -r "DesignToken.*spacing\|object DesignToken" --include="*.kt" | head -30Repository: openMF/android-client
Length of output: 6145
🏁 Script executed:
#!/bin/bash
# Get content of the core spacing definition files
find . -name "*Spacing*" -type f | head -10Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Examine the DesignToken.spacing definition
echo "=== DesignToken.spacing tokens ==="
cat core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
echo ""
echo "=== KptSpacing definition ==="
find . -name "KptSpacing*" -type fRepository: openMF/android-client
Length of output: 16721
🏁 Script executed:
#!/bin/bash
# Find and examine KptSpacing implementation
find . -path "*/designsystem/*" -name "*.kt" -exec grep -l "class KptSpacing\|interface KptSpacing" {} \;Repository: openMF/android-client
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Check what tokens KptTheme.spacing has
grep -r "class KptSpacingImpl\|interface KptSpacing" --include="*.kt" -A 50 | head -100Repository: openMF/android-client
Length of output: 11966
Inconsistent spacing systems: DesignToken.spacing mixed with KptTheme.spacing.
Line 290 uses DesignToken.spacing.dp44 while line 291 uses KptTheme.spacing.md. Similarly, line 795 uses DesignToken.spacing.largeIncreased. The issue is that KptTheme.spacing only provides six tokens (xs, sm, md, lg, xl, xxl) and lacks equivalents for dp44 and largeIncreased. Before this file can fully migrate to KptTheme, either add the missing tokens to KptTheme.spacing or standardize on the closest available alternatives.
Additionally, line 844 uses hardcoded 18.dp—use DesignToken.sizes.dp18 instead.
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt`
around lines 290 - 291, The code mixes DesignToken and KptTheme spacing systems
(e.g., DesignToken.spacing.dp44 vs KptTheme.spacing.md, and
DesignToken.spacing.largeIncreased) and also uses a hardcoded 18.dp; standardize
by either adding the missing tokens to KptTheme (add dp44 and largeIncreased to
KptTheme.spacing and add dp18 to KptTheme.sizes) or replace usages with the
closest KptTheme tokens (e.g., map dp44 → KptTheme.spacing.xxl or another agreed
token, map largeIncreased → KptTheme.spacing.xxl/xl as appropriate) and replace
hardcoded 18.dp with DesignToken.sizes.dp18 or KptTheme.sizes.dp18; update the
references DesignToken.spacing.dp44, DesignToken.spacing.largeIncreased,
KptTheme.spacing.md, and the hardcoded 18.dp so the file consistently uses the
same spacing/sizes system.
| import org.jetbrains.compose.resources.painterResource | ||
| import org.jetbrains.compose.resources.stringResource | ||
| import org.koin.compose.viewmodel.koinViewModel | ||
| import template.core.base.designsystem.theme.KptTheme |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for KptTheme definition to verify the actual package
find . -name "KptTheme.kt" -type f 2>/dev/null | head -5
echo "---"
# Check imports of KptTheme across the codebase
rg "import.*KptTheme" --type=kt | head -30
echo "---"
# Check if template.core.base exists as a package
find . -type d -path "*template/core/base*" 2>/dev/null | head -10Repository: openMF/android-client
Length of output: 917
🏁 Script executed:
#!/bin/bash
# Read the KptTheme.kt file to see the actual package declaration
head -10 ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptTheme.kt
echo "---"
# Check if there's a theme subdirectory under designsystem
find ./core-base/designsystem -type d -name "theme" 2>/dev/nullRepository: openMF/android-client
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Check what's inside the theme subdirectory
ls -la ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/
echo "---"
# Read the actual imports in the file being reviewed
head -80 ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt | grep -E "^import"Repository: openMF/android-client
Length of output: 3271
🏁 Script executed:
#!/bin/bash
# Search for all imports of KptTheme across the codebase
rg "import.*KptTheme" --type=kt
echo "---"
# Double-check the exact file path and confirm KptTheme is not in theme subdirectory
find ./core-base/designsystem -name "KptTheme.kt" -exec echo "File: {}" \; -exec head -15 {} \;Repository: openMF/android-client
Length of output: 799
🏁 Script executed:
#!/bin/bash
# Search with simpler pattern for the import statement
rg "template.core.base.designsystem.theme.KptTheme"
echo "---"
# Try searching just for lines with KptTheme imports
rg "KptTheme" --type=kt -A 0 -B 0 | head -20Repository: openMF/android-client
Length of output: 21633
🏁 Script executed:
#!/bin/bash
# Read KptMaterialTheme.kt to see how it uses the theme imports
head -30 ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptMaterialTheme.kt
echo "---"
# Check if there's a public API or re-export from the theme package
cat ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/KptColorSchemeImpl.kt | head -20Repository: openMF/android-client
Length of output: 2179
🏁 Script executed:
#!/bin/bash
# Verify the exact package declaration more carefully
rg "^package template" ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptTheme.kt
echo "---"
# Check if there are multiple KptTheme.kt files
find . -name "KptTheme.kt" -type f
echo "---"
# Verify imports work by checking the theme subdirectory for any exports
ls -la ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/ | grep -E "\.kt$"Repository: openMF/android-client
Length of output: 275
Fix the package declaration of KptTheme.kt or move it to the theme subdirectory.
The file KptTheme.kt declares its package as template.core.base.designsystem, but all 100+ imports across the codebase (including in KptMaterialTheme.kt) expect it at template.core.base.designsystem.theme.KptTheme. This will cause a compilation error. Either move the file to the theme subdirectory and change its package declaration to template.core.base.designsystem.theme, or update all imports to remove the .theme component. Since the pattern is consistent across the codebase, the file should be in the theme package.
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt`
at line 60, KptTheme.kt currently declares package
template.core.base.designsystem but all consumers (e.g., imports in
KptMaterialTheme.kt and other files) expect
template.core.base.designsystem.theme; move KptTheme.kt into the theme
subdirectory and change its package declaration to
template.core.base.designsystem.theme (or alternatively update all imports to
remove .theme, but prefer moving the file to keep the existing import pattern
consistent).
| tint = KptTheme.colorScheme.primary, | ||
| modifier = Modifier.size(DesignToken.sizes.iconMedium), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the ClientSignatureScreen.kt file
find . -name "ClientSignatureScreen.kt" -type f | head -5Repository: openMF/android-client
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Search for KptTheme definition to understand what tokens it exposes
find . -name "*.kt" -type f -path "*/theme/*" | grep -i "kpttheme" | head -10Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for DesignToken definition to understand size tokens
find . -name "*.kt" -type f | xargs grep -l "object DesignToken\|class DesignToken" | head -5Repository: openMF/android-client
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Read the ClientSignatureScreen.kt file to see imports and the specific lines
cat -n ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt | head -80Repository: openMF/android-client
Length of output: 4525
🏁 Script executed:
#!/bin/bash
# Read lines around 129-130 specifically
sed -n '50,150p' ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt | cat -nRepository: openMF/android-client
Length of output: 4522
🏁 Script executed:
#!/bin/bash
# Read the DesignToken.kt file
cat -n ./core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktRepository: openMF/android-client
Length of output: 20015
🏁 Script executed:
#!/bin/bash
# Search for KptTheme in the codebase more broadly
find . -name "*.kt" -type f -path "*/designsystem/*" | xargs grep -l "KptTheme" | head -10Repository: openMF/android-client
Length of output: 1088
🏁 Script executed:
#!/bin/bash
# Read the KptTheme.kt file to see what properties it exposes
cat -n ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptTheme.ktRepository: openMF/android-client
Length of output: 2981
🏁 Script executed:
#!/bin/bash
# Also check the full ClientSignatureScreen.kt file to see all usages of DesignToken and KptTheme
wc -l ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.ktRepository: openMF/android-client
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Search for other usages of DesignToken.sizes to understand the pattern
rg "DesignToken\.sizes" --type=kt -B2 -A2 | head -50Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check if KptTheme has been extended or if sizes are intentionally separate
find . -name "*.kt" -type f -path "*/designsystem/*" | xargs grep -l "LocalKptSizes\|KptSizes" | head -5Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# See how many Icon() usages exist in ClientSignatureScreen and what tokens they use
sed -n '1,296p' ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt | grep -n "Icon\|Modifier.size\|DesignToken\|KptTheme" | head -30Repository: openMF/android-client
Length of output: 1859
🏁 Script executed:
#!/bin/bash
# Check the KptTheme spacing values to understand the lg value
find . -name "*.kt" -type f -path "*/designsystem/*" | xargs grep -l "KptThemeProvider\|spacing" | head -5Repository: openMF/android-client
Length of output: 568
🏁 Script executed:
#!/bin/bash
# Look at the KptThemeProvider to understand spacing tokens
find . -name "KptThemeProvider*" -type fRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for spacing definitions in KptTheme
rg "spacing\s*=" --type kotlin -A3 -B1 2>/dev/null | head -50Repository: openMF/android-client
Length of output: 4071
🏁 Script executed:
#!/bin/bash
# Find the KptSpacing definition to understand spacing values
find . -name "*.kt" -path "*/designsystem/*" | xargs grep -l "class KptSpacing\|data class KptSpacing" | head -3Repository: openMF/android-client
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Get the actual implementation of KptSpacing
find . -name "KptSpacing*" -type fRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check what KptTheme.spacing.lg and other spacing values are
rg "KptSpacing|val lg|val md|val xs|val xxl" --type kotlin -A5 -B1 2>/dev/null | head -80Repository: openMF/android-client
Length of output: 10163
🏁 Script executed:
#!/bin/bash
# Verify line 247 context in detail
sed -n '240,250p' ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt | cat -nRepository: openMF/android-client
Length of output: 646
Use DesignToken.sizes for icon sizing, not spacing tokens.
Line 247 uses KptTheme.spacing.lg for icon sizing, which is semantically incorrect. While the numeric value happens to be the same (24.dp), spacing tokens are intended for padding and layout gaps, not component dimensions. Use DesignToken.sizes.iconMedium instead to match the pattern on line 130 and maintain semantic clarity.
Show location
Line 247: modifier = Modifier.size(KptTheme.spacing.lg)
Should be: modifier = Modifier.size(DesignToken.sizes.iconMedium)
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt`
around lines 129 - 130, In ClientSignatureScreen replace the incorrect spacing
token used for icon dimensions: change the Modifier.size(...) call that
currently uses KptTheme.spacing.lg to use DesignToken.sizes.iconMedium instead
(match the existing pattern used where tint = KptTheme.colorScheme.primary and
modifier = Modifier.size(DesignToken.sizes.iconMedium)); update the single
Modifier.size invocation to use DesignToken.sizes.iconMedium so the icon uses
the semantic size token rather than a spacing token.
| modifier = Modifier.weight(1f), | ||
| ) | ||
| Spacer(Modifier.padding(DesignToken.padding.small)) | ||
| Spacer(Modifier.padding(KptTheme.spacing.sm)) |
There was a problem hiding this comment.
Spacer with .padding() is semantically incorrect for horizontal spacing in a Row.
Modifier.padding(sm) applies padding on all four sides of a zero-size composable, effectively producing 2 × sm of horizontal space. Use Modifier.width(...) instead for explicit and predictable horizontal spacing.
Proposed fix
- Spacer(Modifier.padding(KptTheme.spacing.sm))
+ Spacer(Modifier.width(KptTheme.spacing.sm))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Spacer(Modifier.padding(KptTheme.spacing.sm)) | |
| Spacer(Modifier.width(KptTheme.spacing.sm)) |
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientStaff/ClientStaffScreen.kt`
at line 141, In ClientStaffScreen.kt replace the zero-size Spacer using
Modifier.padding(KptTheme.spacing.sm) (used inside the Row) with an explicit
horizontal spacer using Modifier.width(KptTheme.spacing.sm) so the spacing is
predictable and semantic; update the Spacer invocation in the Row where it
currently uses Modifier.padding to use a width modifier instead.
| Column(modifier = Modifier.padding(KptTheme.spacing.lg)) { | ||
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(bottom = 16.dp), | ||
| .padding(bottom = KptTheme.spacing.md), | ||
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| Text( | ||
| text = stringResource(Res.string.feature_loan_charge_dialog), | ||
| fontSize = MaterialTheme.typography.titleLarge.fontSize, | ||
| style = KptTheme.typography.titleLarge, | ||
| ) | ||
| IconButton(onClick = { onDismiss() }) { | ||
| Icon( | ||
| imageVector = MifosIcons.Close, | ||
| contentDescription = "", | ||
| modifier = Modifier | ||
| .width(30.dp) | ||
| .height(30.dp), | ||
| .width(KptTheme.spacing.xl) | ||
| .height(KptTheme.spacing.xl), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for dedicated icon-size tokens in the design system
rg -rn 'iconSize|icon_size|IconSize' --type=kt -C2Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Also check for KptTheme definition to understand available tokens
fd -e kt | xargs rg -l 'object KptTheme|class KptTheme' | head -5Repository: openMF/android-client
Length of output: 169
🏁 Script executed:
# Search for other icon width/height usages to understand current patterns
rg -rn 'Icon.*width|Icon.*height' --type=kt -A2 -B2 | head -100Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Look at the KptTheme definition
find . -path "*/designsystem/theme/*.kt" -type f | head -10Repository: openMF/android-client
Length of output: 988
🏁 Script executed:
# Search for KptTheme.spacing and icon-related patterns without file type restriction
rg -i 'KptTheme\.(spacing|size|icon)' --max-count=50Repository: openMF/android-client
Length of output: 50378
🏁 Script executed:
# Search for icon sizing patterns in the specific file mentioned in the review
cat feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/LoanChargeDialogScreen.kt | grep -n -A5 -B5 "KptTheme.spacing.xl"Repository: openMF/android-client
Length of output: 725
🏁 Script executed:
# Check the DesignToken.kt file for available tokens
head -150 core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktRepository: openMF/android-client
Length of output: 5463
🏁 Script executed:
# Check if there's a specific theme configuration that defines spacing vs sizing
head -100 core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/KptColorSchemeImpl.ktRepository: openMF/android-client
Length of output: 4465
🏁 Script executed:
# Search for specific icon sizing patterns to understand the design system intent
rg 'size|Size' core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/ -A2 -B2 | head -80Repository: openMF/android-client
Length of output: 7742
🏁 Script executed:
# Check KptTheme for sizes token exposure
rg 'object KptTheme|interface KptTheme|val sizes' core-base/designsystem/ -A5 -B2 | head -60Repository: openMF/android-client
Length of output: 2072
🏁 Script executed:
# Look for KptSpacing and any related sizing definitions
cat core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/core/KptThemeProvider.kt 2>/dev/null | head -100Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Get the full KptTheme definition
cat core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/KptColorSchemeImpl.kt | grep -A 20 "object KptTheme"Repository: openMF/android-client
Length of output: 628
🏁 Script executed:
# Check for KptSpacing definition
fd -name "*.kt" -path "*/designsystem/*" | xargs grep -l "class KptSpacing\|data class KptSpacing" 2>/dev/nullRepository: openMF/android-client
Length of output: 297
🏁 Script executed:
# Search for icon sizing patterns in the codebase more broadly
rg 'Icon.*width|Icon.*height|\.size\(' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/ -B2 -A2Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check if AppSizes is defined and what it contains
rg 'class AppSizes|data class AppSizes' core/designsystem/ -A 20Repository: openMF/android-client
Length of output: 2654
🏁 Script executed:
# Search for .size() usage in icon contexts across the codebase
rg '\.size\(.*spacing' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/ -B2 -A2Repository: openMF/android-client
Length of output: 1434
🏁 Script executed:
# Check if there's any pattern of icon sizing without spacing
rg 'Icon\(' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/ -A10 | head -40Repository: openMF/android-client
Length of output: 3308
Use DesignToken.sizes.iconLarge instead of spacing token for icon dimensions.
Lines 202–203 use KptTheme.spacing.xl to set the close-icon's width and height. The design system provides dedicated sizing tokens in DesignToken.sizes (e.g., iconLarge: 32.dp, iconExtraLarge: 36.dp) that are semantically appropriate for icon dimensions. Replace KptTheme.spacing.xl (28.dp) with the closest matching icon size, such as DesignToken.sizes.iconLarge (32.dp).
Current code (lines 202–203):
.width(KptTheme.spacing.xl)
.height(KptTheme.spacing.xl),
Note: KptTheme currently exposes only spacing, not sizes. Use DesignToken.sizes directly, or consider exposing the sizes token through KptTheme to provide a consistent API.
🤖 Prompt for AI Agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/LoanChargeDialogScreen.kt`
around lines 185 - 205, The close Icon in LoanChargeDialogScreen (inside the
Column/Row -> IconButton -> Icon using MifosIcons.Close) is sized with
KptTheme.spacing.xl; replace those width/height uses with the semantically
correct design token DesignToken.sizes.iconLarge (or iconExtraLarge if you
prefer 36.dp) so the icon uses the dedicated size tokens instead of spacing;
optionally, if you want a consistent theme API, expose sizes via KptTheme (e.g.,
KptTheme.sizes.iconLarge) and use that in the Icon modifier.
| import com.mifos.feature.loan.newLoanAccount.NewLoanAccountAction | ||
| import com.mifos.feature.loan.newLoanAccount.NewLoanAccountState | ||
| import org.jetbrains.compose.resources.stringResource | ||
| import template.core.base.designsystem.theme.KptTheme |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find KptTheme.kt file to check its actual package
fd "KptTheme.kt" --type fRepository: openMF/android-client
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Also search for KptTheme references and its package definition
rg "^package " --type kt | rg -i "kptheme|template"Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Get the file size and then read it
wc -l feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.ktRepository: openMF/android-client
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Read lines around 95 to verify the import
sed -n '90,100p' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.ktRepository: openMF/android-client
Length of output: 558
🏁 Script executed:
#!/bin/bash
# Read lines around 559 to check the isCheckedEqualAmortization issue
sed -n '555,565p' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.ktRepository: openMF/android-client
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Read lines around 716 to check spacing vs icon size issue
sed -n '710,720p' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.ktRepository: openMF/android-client
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Check a larger sample to see the pattern of inline comments
sed -n '100,150p' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.ktRepository: openMF/android-client
Length of output: 2024
Fix incorrect checkbox state toggle at line 559 and semantic mismatch for icon size at line 716
Line 559: The clickable row for "interest partial period" toggles !state.isCheckedEqualAmortization but should toggle !state.isCheckedInterestPartialPeriod to match the checkbox state being read.
Line 716: Icon size is set using KptTheme.spacing.md, but spacing tokens are for margins/padding, not sizes. This should use a proper size token (the comment indicates DesignToken.sizes.iconSmall was replaced—restore that or use an equivalent size token).
🤖 Prompt for AI Agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt`
at line 95, Fix the checkbox toggle and icon size: in the clickable row handler
that currently negates state.isCheckedEqualAmortization, change it to toggle
state.isCheckedInterestPartialPeriod so the row toggles the same boolean the
Checkbox reads (reference state.isCheckedInterestPartialPeriod and the clickable
lambda in TermsPage). Also replace the icon size usage of KptTheme.spacing.md
with a proper size token (restore or use DesignToken.sizes.iconSmall or an
equivalent icon size token) where the Icon is created so spacing tokens are not
used for icon sizing.
| Text( | ||
| text = stringResource(Res.string.feature_recurring_deposit_step_details), | ||
| style = MifosTypography.labelLarge, | ||
| // TYPOGRAPHY: Mapped to KptTheme | ||
| style = KptTheme.typography.labelLarge, | ||
| // COLOR: Added for theme consistency | ||
| color = KptTheme.colorScheme.onSurface, | ||
| ) |
There was a problem hiding this comment.
Typography inconsistency with sibling pages.
Section header Text composables here use KptTheme.typography.labelLarge, while TermsPage and SettingsPage use KptTheme.typography.titleMedium for equivalent section headers (e.g., "Step Terms", "Step Settings"). If the intent is for all step headers to look the same, this should be titleMedium as well.
🤖 Prompt for AI Agents
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt`
around lines 89 - 95, The section header Text in PreviewPage (the Text
composable using KptTheme.typography.labelLarge) must be changed to use
KptTheme.typography.titleMedium to match the sibling pages (TermsPage and
SettingsPage) so step headers are consistent; update the style reference in
PreviewPage.kt from labelLarge to titleMedium and keep the existing color
(KptTheme.colorScheme.onSurface) unchanged.
| if (fundList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedFund, | ||
| onValueChanged = { selectedFund = it }, | ||
| onOptionSelected = { index, value -> | ||
| selectedFund = value | ||
| selectedFundId = fundList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_fund), | ||
| options = fundList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| } | ||
|
|
||
| if (currencyList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedCurrency, | ||
| onValueChanged = { | ||
| selectedCurrency = it | ||
| }, | ||
| onOptionSelected = { index, value -> | ||
| selectedCurrency = value | ||
| selectedCurrencyId = currencyList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_currency), | ||
| options = currencyList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| } | ||
| if (currencyList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedCurrency, | ||
| onValueChanged = { selectedCurrency = it }, | ||
| onOptionSelected = { index, value -> | ||
| selectedCurrency = value | ||
| selectedCurrencyId = currencyList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_currency), | ||
| options = currencyList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| } | ||
|
|
||
| if (parCalculatorList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedParCalculator, | ||
| onValueChanged = { | ||
| selectedParCalculator = it | ||
| }, | ||
| onOptionSelected = { index, value -> | ||
| selectedParCalculator = value | ||
| selectedParCalculatorId = parCalculatorList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_par_type), | ||
| options = parCalculatorList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| } | ||
| if (parCalculatorList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedParCalculator, | ||
| onValueChanged = { selectedParCalculator = it }, | ||
| onOptionSelected = { index, value -> | ||
| selectedParCalculator = value | ||
| selectedParCalculatorId = parCalculatorList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_par_type), | ||
| options = parCalculatorList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| } | ||
|
|
||
| if (savingsAccountDepositList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedSavingsAccountDeposit, | ||
| onValueChanged = { | ||
| selectedSavingsAccountDeposit = it | ||
| }, | ||
| onOptionSelected = { index, value -> | ||
| selectedSavingsAccountDeposit = value | ||
| selectedSavingsAccountDepositId = savingsAccountDepositList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_saving_account), | ||
| options = savingsAccountDepositList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| } | ||
| if (savingsAccountDepositList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedSavingsAccountDeposit, | ||
| onValueChanged = { selectedSavingsAccountDeposit = it }, | ||
| onOptionSelected = { index, value -> | ||
| selectedSavingsAccountDeposit = value | ||
| selectedSavingsAccountDepositId = savingsAccountDepositList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_saving_account), | ||
| options = savingsAccountDepositList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| } | ||
|
|
||
| if (glAccountList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedGlAccount, | ||
| onValueChanged = { | ||
| selectedGlAccount = it | ||
| }, | ||
| onOptionSelected = { index, value -> | ||
| selectedGlAccount = value | ||
| selectedGlAccountId = glAccountList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_gl_account), | ||
| options = glAccountList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| } | ||
| if (glAccountList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedGlAccount, | ||
| onValueChanged = { selectedGlAccount = it }, | ||
| onOptionSelected = { index, value -> | ||
| selectedGlAccount = value | ||
| selectedGlAccountId = glAccountList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_gl_account), | ||
| options = glAccountList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| } | ||
|
|
||
| if (obligationDateList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedObligationDate, | ||
| onValueChanged = { | ||
| selectedObligationDate = it | ||
| }, | ||
| onOptionSelected = { index, value -> | ||
| selectedObligationDate = value | ||
| selectedObligationDateId = obligationDateList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_obligation_date), | ||
| options = obligationDateList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| if (obligationDateList.isNotEmpty()) { | ||
| MifosTextFieldDropdown( | ||
| value = selectedObligationDate, | ||
| onValueChanged = { selectedObligationDate = it }, | ||
| onOptionSelected = { index, value -> | ||
| selectedObligationDate = value | ||
| selectedObligationDateId = obligationDateList[index].row.first() ?: "" | ||
| }, | ||
| label = stringResource(Res.string.feature_report_obligation_date), | ||
| options = obligationDateList.mapNotNull { it.row.getOrNull(1) }, | ||
| readOnly = true, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent safe-access pattern across dropdowns.
The first four dropdowns (office, loanPurpose, loanOfficer, products) use safe access:
officeList.getOrNull(index)?.row?.getOrNull(0) ?: ""but the remaining six (fund, currency, parCalculator, savings, glAccount, obligationDate) use unsafe direct indexing:
fundList[index].row.first() ?: ""list[index] throws IndexOutOfBoundsException and .first() throws NoSuchElementException on empty rows. Use getOrNull consistently for all dropdowns.
Example fix for the fund dropdown (apply same pattern to all six)
onOptionSelected = { index, value ->
selectedFund = value
- selectedFundId = fundList[index].row.first() ?: ""
+ selectedFundId = fundList.getOrNull(index)?.row?.getOrNull(0) ?: ""
},🤖 Prompt for AI Agents
In
`@feature/report/src/commonMain/kotlin/com/mifos/feature/report/reportDetail/ReportDetailScreen.kt`
around lines 491 - 573, The onOptionSelected handlers for MifosTextFieldDropdown
using fundList, currencyList, parCalculatorList, savingsAccountDepositList,
glAccountList, and obligationDateList use unsafe indexing (e.g.,
fundList[index].row.first()) which can throw; update each handler (the
onOptionSelected lambdas in the MifosTextFieldDropdown for selectedFund,
selectedCurrency, selectedParCalculator, selectedSavingsAccountDeposit,
selectedGlAccount, selectedObligationDate) to use safe-access like
getOrNull(index)?.row?.getOrNull(0) ?: "" so you consistently mirror the safe
pattern used elsewhere (officeList, loanPurpose, loanOfficer, products).
| .border( | ||
| width = 2.dp, | ||
| color = MaterialTheme.colorScheme.primary, | ||
| shape = RoundedCornerShape(0.dp), | ||
| width = KptTheme.spacing.xs, | ||
| color = KptTheme.colorScheme.primary, | ||
| shape = RectangleShape, | ||
| ) |
There was a problem hiding this comment.
KptTheme.spacing.xs is almost certainly too large for a border width.
Spacing tokens (xs is typically 4.dp+) are designed for layout gaps and padding, not stroke widths. This will render as a very thick border around each dialog row, which is likely a visual regression. The previous value was probably 1.dp.
Use a fixed small Dp value (e.g., 1.dp) or introduce a dedicated border-width token in the design system instead of repurposing a spacing token.
Proposed fix
+import androidx.compose.ui.unit.dp
...
.border(
- width = KptTheme.spacing.xs,
+ width = 1.dp,
color = KptTheme.colorScheme.primary,
shape = RectangleShape,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .border( | |
| width = 2.dp, | |
| color = MaterialTheme.colorScheme.primary, | |
| shape = RoundedCornerShape(0.dp), | |
| width = KptTheme.spacing.xs, | |
| color = KptTheme.colorScheme.primary, | |
| shape = RectangleShape, | |
| ) | |
| .border( | |
| width = 1.dp, | |
| color = KptTheme.colorScheme.primary, | |
| shape = RectangleShape, | |
| ) |
🤖 Prompt for AI Agents
In
`@feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt`
around lines 517 - 521, The border width is using a spacing token
(KptTheme.spacing.xs) which is too large for strokes; in the
SavingsAccountSummaryScreen composable replace the width argument on the .border
call (currently using KptTheme.spacing.xs) with a small fixed Dp (e.g., 1.dp) or
a dedicated border token (for example KptTheme.border.small) and ensure the
appropriate import for dp is present; update the .border invocation in
SavingsAccountSummaryScreen to use that small stroke value instead of the
spacing token.
1 participant
Fixes - Jira-#603
Screenshots:
Summary by CodeRabbit
New Features
Refactor