refactor(feature): replace MaterialTheme with kptTheme#2610
refactor(feature): replace MaterialTheme with kptTheme#2610kartikey004 wants to merge 1 commit intoopenMF:developmentfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates runtime dependency classpaths (Compose Material3 adaptive and androidx.window 1.3.x), exposes the designsystem module via an API dependency, adds many design token values, migrates hundreds of composables from MaterialTheme/legacy tokens to KptTheme/DesignToken tokens, and applies a few composable signature/type changes. Changes
Sequence Diagram(s)(omitted — changes are primarily presentation, dependency and minor signature edits) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
Show resolved
Hide resolved
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
Show resolved
Hide resolved
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Show resolved
Hide resolved
...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
...in/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt
Outdated
Show resolved
Hide resolved
...n/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt
Show resolved
Hide resolved
...Main/kotlin/com/mifos/feature/savings/savingsAccountApproval/SavingsAccountApprovalScreen.kt
Show resolved
Hide resolved
...ure/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt
Outdated
Show resolved
Hide resolved
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.
...rc/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInbox/CheckerInboxScreen.kt
Show resolved
Hide resolved
...e/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt
Show resolved
Hide resolved
...monMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt
Show resolved
Hide resolved
...ient/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt
Show resolved
Hide resolved
| 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.
...loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/LoanChargeDialogScreen.kt
Outdated
Show resolved
Hide resolved
| 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.
...in/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt
Show resolved
Hide resolved
| 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).
...onMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt
Show resolved
Hide resolved
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
7c0ebc4 to
f5f7e4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/groupLoanAccount/GroupLoanAccountScreen.kt (1)
296-300:⚠️ Potential issue | 🟠 MajorPre-existing bug: disbursement date picker dismiss sets wrong flag.
Line 298 sets
showSubmissionDatePicker = falseinstead ofshowDisbursementDatePicker = false. This means dismissing the disbursement date picker won't actually close it—it will toggle the wrong picker's state. Not introduced by this PR, but worth fixing while you're in 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/groupDetails/GroupDetailsScreen.kt (2)
494-514:⚠️ Potential issue | 🟠 MajorUnreachable branch:
active && inArrearsis shadowed by the earlieractivecheck.The
active == truecondition on line 495 matches first, so theactive == true && inArrears == truebranch on line 507 can never execute. The arrears case should be checked before the plain active case.🐛 Proposed fix
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 } },
594-596: 🛠️ Refactor suggestion | 🟠 MajorHardcoded
24.dpnot migrated to design token.Line 432 uses
DesignToken.sizes.iconMediumfor the sameIconButton.size()pattern inMifosLoanAccountExpendableCard, but here inMifosSavingsAccountExpendableCardit's still a raw literal.♻️ Proposed fix
IconButton( modifier = Modifier - .size(24.dp), + .size(DesignToken.sizes.iconMedium), onClick = { expendableState = !expendableState },feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (1)
168-201:⚠️ Potential issue | 🟠 MajorCurrency symbol prepended to "Not Available" produces malformed text like
$Not Available.When
originalLoanis null,amountPaidis pending, orloanBalanceis pending, the symbol is still prepended, producing strings like"$Not Available"or"₹Not Available".The symbol should only be included when displaying an actual numeric value.
Proposed fix (showing originalLoan; 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(lines 177–188):- 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() + },Apply the same pattern for
loanBalance.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt (1)
199-209:⚠️ Potential issue | 🟡 MinorUnreachable branch:
Feature.VIEW_DOCUMENTinside a guard that excludes it.Line 199 checks
state.feature != Feature.VIEW_DOCUMENT, so thestate.feature == Feature.VIEW_DOCUMENTbranch at line 202 can never be true. This is dead code.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/clientAddress/addAddress/AddAddressScreen.kt (1)
534-573:⚠️ Potential issue | 🟠 MajorPre-existing bug:
validateAddressFieldsreturns before coroutine completes.
scope.launch { ... }is asynchronous, but the function returnsresultimmediately on line 572 — before the coroutine body on lines 544–570 has executed. This means validation errors are never actually propagated; the caller always receives an emptyAddressValidationResult(all fieldsnull,isValid == true).This is not introduced by this PR, but it means submit-button validation is effectively a no-op. Consider making this a
suspendfunction with direct assignment instead of launching a coroutine, or using a callback/state-flow pattern.feature/report/src/commonMain/kotlin/com/mifos/feature/report/runReport/RunReportScreen.kt (1)
336-348:⚠️ Potential issue | 🟠 MajorWrap
RunReportPreviewwithKptTheme.
RunReportScreenreferencesKptTheme.typography,KptTheme.colorScheme, andKptTheme.spacingthroughout its implementation. Without aKptTheme { }wrapper providing the themeCompositionLocal, the preview will fail or render with fallback defaults.Suggested fix
`@Preview` `@Composable` private fun RunReportPreview( `@PreviewParameter`(RunReportUiStateProvider::class) state: RunReportUiState, ) { + KptTheme { RunReportScreen( state = state, onBackPressed = {}, onMenuClick = {}, onRetry = {}, onReportClick = {}, ) + } }feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
835-856:⚠️ Potential issue | 🔴 CriticalReplace
MifosThemewithKptMaterialThemein the preview wrapper.The preview wraps content in
MifosTheme, but composables useKptThemefor colors, typography, shapes, and spacing.MifosThemeonly providesMaterialThemeandDesignTokenTheme(which has different composition locals likeLocalSpacing, notLocalKptSpacing), so the preview will render with fallback/default values instead of the intended theme tokens.Use
KptMaterialThemeinstead, which provides bothMaterialThemeand the requiredKptThemecomposition locals.feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/createNewGroup/CreateNewGroupScreen.kt (1)
342-362:⚠️ Potential issue | 🟡 MinorPre-existing layout issue:
AnimatedVisibilitywraps content in aBox, so theSpacerandMifosDatePickerTextFieldoverlap.
AnimatedVisibilityuses aBoxlayout internally. TheSpacerat Line 353 andMifosDatePickerTextFieldat Line 355 are siblings inside thatBox, so they'll overlap rather than stack vertically. Consider wrapping them in aColumn, or move theSpaceroutside theAnimatedVisibilityblock.This is a pre-existing issue, not introduced by this PR, but worth addressing.
🔧 Proposed fix
) { - Spacer(modifier = Modifier.height(KptTheme.spacing.md)) - - MifosDatePickerTextField( - value = DateHelper.getDateAsStringFromLong(activationDate), - label = stringResource(Res.string.feature_groups_activation_date), - openDatePicker = { - activationDatePicker = true - }, - ) + 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/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt (1)
238-397:⚠️ Potential issue | 🔴 CriticalFix non-existent elevation property
level1— useelevationinstead.
KptTheme.elevation.level1does not exist. TheAppElevationclass only providesnone,appBar, andelevationproperties. UseKptTheme.elevation.elevation(25.dp) for the card elevation, which aligns with the context of elevated UI surfaces.feature/search-record/src/commonMain/kotlin/com/mifos/feature/searchrecord/SearchRecordScreen.kt (1)
446-458:⚠️ Potential issue | 🟡 MinorPreview still wraps content in
MaterialThemeinstead ofKptTheme.The preview function on line 451 still uses
MaterialTheme { … }, which is inconsistent with the refactor. Replace it withKptTheme { … }and remove the now-unusedMaterialThemeimport on line 46.Proposed fix
- MaterialTheme { + KptTheme { SearchRecordScreen( state = state, onBackClick = {}, onRecordSelected = {}, ) }And remove the unused import:
-import androidx.compose.material3.MaterialTheme
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt`:
- Around line 523-528: The inner Card color is inconsistent between
MifosLoanAccountsLazyColumn and MifosSavingsAccountsLazyColumn—one uses
KptTheme.colorScheme.secondary while the other uses
KptTheme.colorScheme.surface; update the CardDefaults.cardColors call in the
Card inside MifosLoanAccountsLazyColumn (or the one in
MifosSavingsAccountsLazyColumn if you prefer the secondary tone) so both list
containers use the same KptTheme.colorScheme value (e.g., change the Card in
MifosLoanAccountsLazyColumn to use KptTheme.colorScheme.surface) to make the two
structurally identical list cards visually consistent.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt`:
- Around line 286-289: The dialog header's font weight was changed from
MifosTypography.titleMediumEmphasized (SemiBold) to
KptTheme.typography.titleMedium (Medium); confirm whether this visual
de-emphasis in CreateShareAccountScreen's Text is intentional, and if not
restore the previous emphasis by either switching back to
MifosTypography.titleMediumEmphasized or by applying a SemiBold weight to
KptTheme.typography.titleMedium (e.g., copy the style and set
FontWeight.SemiBold) so the header retains the original emphasis.
In
`@feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/generateCollectionSheet/GenerateCollectionSheetScreen.kt`:
- Line 303: Replace the Spacer usages that call
Modifier.heightIn(KptTheme.spacing.md) with Modifier.height(KptTheme.spacing.md)
to enforce a fixed spacer height (do this for all seven occurrences in
GenerateCollectionSheetScreen.kt); locate these Spacers inside the
GenerateCollectionSheetScreen composable (and any inner composables related to
collection sheet generation) and update each Modifier.heightIn(...) to
Modifier.height(...) so the spacer uses a fixed height consistent with other
screens like CreateNewGroupScreen and CenterDetailsScreen.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/component/RepaymentPeriodCard.kt`:
- Around line 62-73: The modifier chain in RepaymentPeriodCard currently applies
a redundant .border(...) modifier while the MifosCard call also supplies
borderStroke; remove the .border(...) call from the modifier passed into
MifosCard (keep modifier.fillMaxWidth()) and rely on the existing borderStroke
parameter and shape (KptTheme.shapes.medium) to render the card border, ensuring
you leave containerColor, shape, and borderStroke unchanged.
🧹 Nitpick comments (35)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientTransfer/ClientTransferScreen.kt (1)
217-217: Mixed token systems:DesignToken.sizesstill used alongsideKptTheme.Lines 217 and 239 use
DesignToken.sizes.iconAveragewhile the rest of the file has migrated toKptThemetokens. IfKptThemeexposes equivalent size tokens, consider migrating these for consistency. If size tokens aren't part ofKptThemeyet, this is fine as-is.Also applies to: 239-239
feature/client/src/commonMain/kotlin/com/mifos/feature/client/documentPreviewScreen/DocumentPreviewScreen.kt (1)
216-218: Inconsistent border width token: hardcoded1.dpvsDesignToken.spacing.dp1.Lines 115 and 143 use
DesignToken.spacing.dp1for the border width, but line 218 still uses a hardcoded1.dp. For consistency within this file, consider using the same token.Proposed fix
- borderStroke = BorderStroke(1.dp, KptTheme.colorScheme.secondaryContainer), + borderStroke = BorderStroke(DesignToken.spacing.dp1, KptTheme.colorScheme.secondaryContainer),feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt (1)
76-76: ResidualDesignToken.sizes.iconSmall— consider migrating to KptTheme for consistency.This is the only remaining
DesignTokenreference in the file (line 37 import kept solely for this). If KptTheme exposes an equivalent size token, migrating it would allow removing theDesignTokenimport entirely.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/ClientAddressScreen.kt (1)
228-253: MixingDesignToken.spacing.dp1withKptThemetokens — intentional?Line 231 uses
DesignToken.spacing.dp1for the border width while all other spacing/styling references have been migrated toKptTheme. IfKptThemedoesn't expose adp1equivalent, this is fine and consistent with the team's raw dp-based naming preference for design tokens. Just confirming this is the intended pattern for raw dimension values that don't have a semantic KptTheme counterpart.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanApproval/LoanAccountApprovalScreen.kt (1)
304-308: ClarifyheightInintent with a named parameter.
.heightIn(DesignToken.sizes.buttonHeightMedium)works because the first positional argument maps tomin, but using the named parameter makes the intent explicit and avoids ambiguity withmax.Proposed clarity improvement
- .heightIn(DesignToken.sizes.buttonHeightMedium), + .heightIn(min = DesignToken.sizes.buttonHeightMedium),feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccount/SavingsAccountScreen.kt (1)
467-504: Mixed usage ofDesignToken.spacingandKptTheme.spacingon adjacent lines.Line 470 uses
DesignToken.spacing.dp44for the button min-height while Line 471 usesKptTheme.spacing.mdfor padding. IfKptThemeexposes a sizes/spacing token fordp44, prefer usingKptThemeconsistently. If not, consider whetherdp44belongs under asizestoken rather thanspacing, since it represents a component height, not whitespace.This is a minor consistency nit — feel free to defer if
KptThemedoesn't yet expose an equivalent size token.#!/bin/bash # Check if KptTheme exposes a sizes or equivalent token that includes dp44 ast-grep --pattern 'object KptTheme { $$$ }' # Also check what tokens DesignToken.spacing and any AppSizes expose rg -n 'dp44' --type=kt -C2feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt (2)
132-133: Remove the refactoring breadcrumb comment.This comment documents what changed, not why. It reads as a commit note rather than a useful code comment and will become noise after the PR is merged.
Suggested diff
- // SPACING: Replaced 4.dp with KptTheme.spacing.xs horizontalArrangement = Arrangement.spacedBy(KptTheme.spacing.xs),
100-101: Consider using a size token instead of a spacing token for the indicator dimensions.
KptTheme.spacing.mdis semantically a spacing value. IfDesignToken.sizes(orKptTheme.sizes) exposes a matching 16 dp token, using that would better convey intent —spacingfor whitespace,sizesfor component dimensions.feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupDetails/GroupDetailsScreen.kt (1)
468-471: Fragile fixed-heightLazyColumnwith magic multiplier.
(loanAccounts.size * 52).dpassumes each item is exactly 52dp tall. The savings variant on line 633 uses 50dp per item — an inconsistency that suggests the values are approximations. If font scaling, density, or content changes, items will be clipped or leave excess space. Consider usingModifier.heightIn(max = ...)with a reasonable cap, or wrapping in a non-scrollableColumnsince the outer container already scrolls.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt (2)
431-443: HardcodedColor.Greenis inconsistent with the KptTheme migration.The
errorandonSurfacecolors in this samewhenblock useKptTheme.colorScheme, but the deposit case still usesColor.Green. Consider introducing a semantic token (e.g., a "success" or "positive" color) in the design system, or at minimum use a theme-aware color for consistency.Proposed fix (minimal)
If no success token exists yet, at minimum extract the color to a constant or theme extension so it can be updated in one place later:
transaction.transactionType?.deposit == true -> { - Color.Green + KptTheme.colorScheme.tertiary // or a dedicated success token }
515-523: Border width fix looks good — but accessingDesignTokendirectly breaks the abstraction layer.Using
DesignToken.spacing.dp1fixes the previous issue of an oversized border. However, the rest of this file consistently goes throughKptTheme.spacing.*/KptTheme.colorScheme.*. Reaching intoDesignTokendirectly bypasses the theme indirection and couples this composable to the raw token layer.Consider either exposing
dp1viaKptTheme.spacingor introducing a border-width token (e.g.,KptTheme.border.thin) so this file stays at one abstraction level.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (2)
171-201: Hardcoded"Not Available"strings should be extracted to a string resource.The literal
"Not Available"appears six times in this block. The rest of the file usesstringResource(Res.string.…)for user-facing text. Extract this to a single string resource for consistency and i18n support.
303-303: LeftoverDesignToken.sizes.iconMinyMiny— inconsistent with the KptTheme migration.This is the only remaining
DesignTokenreference in the file (and its import on line 63). Consider migrating this to aKptThemesize token to complete the migration.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountActivate/SavingsAccountActivateScreen.kt (1)
142-144: Embedding theme-derived padding in the defaultmodifieris fragile.When a caller passes a custom
modifier, the horizontal padding is silently lost. The idiomatic Compose approach is to keepModifieras the default and apply internal layout padding inside the composable body.♻️ Suggested refactor: move padding inside the composable
private fun SavingsAccountActivateContent( - modifier: Modifier = Modifier.padding(horizontal = KptTheme.spacing.sm), + modifier: Modifier = Modifier, activateSavings: (hashMap: HashMap<String, String>) -> Unit, ) { ... Column( modifier = modifier + .padding(horizontal = KptTheme.spacing.sm) .fillMaxSize() .verticalScroll(scrollstate), ) {feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt (1)
275-277: No-op.map { it }— just pass the list directly.
state.statusList.map { it }creates a new list identical to the original. Passstate.statusListdirectly.Proposed fix
- options = state.statusList.map { - it - }, + options = state.statusList,feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/paymentDetails/PaymentDetailsScreen.kt (1)
370-378: Same spacing-vs-size token concern for the Cancel/Save button heights.Same issue as above —
DesignToken.spacing.dp50is a spacing token being used to set button height on Lines 371 and 377. Use a size token if available, or remove the explicit height to useMifosButton's built-in 48.dp default.feature/data-table/src/commonMain/kotlin/com/mifos/feature/dataTable/dataTableData/DataTableDataScreen.kt (1)
352-353: Remove the mapping comment — it's a leftover from the migration process.The comment
// TYPOGRAPHY: Mapped to KptTheme titleMediumis an implementation note that doesn't add value for future readers.Suggested fix
- // TYPOGRAPHY: Mapped to KptTheme titleMedium style = KptTheme.typography.titleMedium,feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt (2)
287-293: Incomplete "More" option handler.The
onClickfor the "More" bottom sheet option is a no-op with the comment// it implement further. Consider adding aTODO("MIFOSAC-XXX: ...")so it's trackable, or disable the item visually to avoid confusing users who tap it.Would you like me to open an issue to track this incomplete handler?
198-201:MifosUserSignatureImageincore/uistill usesMaterialThemedirectly.The component (
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosUserSignature.kt) referencesMaterialTheme.colorScheme(lines 44, 50, 70) andMaterialTheme.typography.bodySmall(line 69). For design system consistency, consider updating this component to useKptThemein a follow-up when thecore/uimodule is part of the broader theme migration.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (2)
152-156: Remove the dead privateSelectionModeTopAppBaroverload and its commented-out call site.The private
SelectionModeTopAppBar(lines 232–276) is only referenced by the commented-out block at lines 152–156. Since the active code now delegates to the importedcom.mifos.core.ui.components.SelectionModeTopAppBar, both the commented-out invocation and the unused private function are dead code and should be removed to avoid confusion.♻️ Proposed cleanup
Remove lines 152–156:
-// SelectionModeTopAppBar( -// currentSelectedItems = selectedItems.selectedItems.value, -// syncClicked = { sync.value = true }, -// resetSelectionMode = resetSelectionMode, -// )Remove lines 230–276 (the entire unused private overload):
-@OptIn(ExperimentalMaterial3Api::class) -@Composable -private fun SelectionModeTopAppBar( - currentSelectedItems: List<ClientEntity>, - syncClicked: () -> Unit, - resetSelectionMode: () -> Unit, -) { - val selectedItems = currentSelectedItems.toMutableStateList() - TopAppBar( - colors = TopAppBarDefaults.topAppBarColors( - containerColor = KptTheme.colorScheme.secondary, - ), - title = { - Text( - text = "${selectedItems.size} selected", - style = KptTheme.typography.titleLarge.copy( - color = KptTheme.colorScheme.onBackground, - ), - ) - }, - navigationIcon = { - IconButton( - onClick = resetSelectionMode, - ) { - Icon( - imageVector = MifosIcons.Close, - contentDescription = "Close", - tint = KptTheme.colorScheme.onSurface, - ) - } - }, - actions = { - IconButton( - onClick = { - syncClicked() - resetSelectionMode() - }, - ) { - Icon( - imageVector = MifosIcons.Sync, - contentDescription = "Sync", - tint = KptTheme.colorScheme.onSurface, - ) - } - }, - ) -}This also resolves the color-contrast accessibility concern (using
onBackground/onSurfaceagainst asecondarybackground) that was flagged in a previous review, since the problematic code would simply be deleted.Also applies to: 232-276
329-337: ElevatedCard styling looks good; minor padding simplification possible.The card theming migration to
KptTheme.elevation.level1,KptTheme.colorScheme.surface, andKptTheme.shapes.mediumis consistent. One small nit: at line 332,padding(horizontal = KptTheme.spacing.md, vertical = KptTheme.spacing.md)is equivalent topadding(KptTheme.spacing.md).✏️ Optional simplification
- .padding(horizontal = KptTheme.spacing.md, vertical = KptTheme.spacing.md), + .padding(KptTheme.spacing.md),feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/settings/SettingsScreen.kt (1)
275-287: Wrap preview composable inKptTheme.
SettingsScreenusesKptThemetokens (colors, typography, shapes, spacing) throughout. Without wrapping the preview inKptTheme, accessing these CompositionLocal values will crash at runtime.Proposed fix
`@Composable` `@DevicePreview` private fun PreviewSettingsScreen() { + KptTheme { SettingsScreen( onBackPressed = {}, state = SettingsUiState.DEFAULT, handleEndpointUpdate = { _, _ -> }, updateLanguage = {}, updateTheme = {}, changePasscode = {}, onClickUpdateConfig = {}, ) + } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)
964-972: Minor:DesignToken.spacing.dp2for border width is a spacing token used for a visual property.Similar to the button height concern,
dp2here is a spacing token but is used for border width. This is a very minor semantic mismatch — consider whether a dedicated border-width token or a size token would be more appropriate. That said, for a 2dp border, this is pragmatically fine.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt (2)
525-539: Dead code:if (false)trailing icon blocks.Multiple text fields (Lines 528, 575, 802, 830) use
if (false)for the trailing icon, making these lambdas unreachable. TheisErroris also hardcoded tofalse. If error display is intended for these fields in the future, wire up actual error state; otherwise, remove the dead branches to reduce noise.
703-729: Consider extracting shared text field styling.The identical
colors(...),shape, andtextStyleconfiguration is repeated across ~8MifosOutlinedTextFieldcalls. Extracting these into a composable-scoped default (e.g.,val defaultColors = colors(...)andval defaultShape = KptTheme.shapes.medium) at the top of the parent composable would reduce duplication and make future theme tweaks a single-line change.feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/syncSurvey/SyncSurveysDialog.kt (1)
431-431: Inconsistent token access:DesignToken.spacing.dp40vsKptTheme.spacing.*used elsewhere.Every other spacing reference in this file goes through
KptTheme.spacing, but the button height reads the staticDesignTokenobject directly. IfKptTheme.spacingexposes an equivalentdp40token, prefer it for consistency; otherwise consider adding one.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt (1)
456-516:MifosLoanAccountExpendableCardandMifosSavingsAccountExpandableCardare nearly identical.These two composables share the same expand/collapse Card layout, animation, and icon logic — only the content slot differs. Consider extracting a shared
MifosExpandableCard(title: String, content:@composable() -> Unit)to reduce duplication. Not blocking for a theme-migration PR, but worth a follow-up.Also applies to: 611-671
feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginScreen.kt (1)
174-174: MixingDesignToken.spacingandKptTheme.spacing— intentional but worth a brief note.Lines 174 and 241 use
DesignToken.spacing.dp80andDesignToken.spacing.dp44(raw dp tokens), while the rest of the file usesKptTheme.spacing.*(semantic tokens likemd,sm,xs). This is understandable when no semantic equivalent exists, but a brief inline comment (e.g.,// no semantic token for 80dp) could help future readers quickly understand the rationale.Also applies to: 241-241
feature/document/src/commonMain/kotlin/com/mifos/feature/document/documentList/DocumentListScreen.kt (1)
284-316: Line 285 is quite long — consider breaking the modifier chain for readability.♻️ Suggested formatting
- modifier = modifier.fillMaxWidth().padding(vertical = KptTheme.spacing.xs, horizontal = KptTheme.spacing.sm), + modifier = modifier + .fillMaxWidth() + .padding(vertical = KptTheme.spacing.xs, horizontal = KptTheme.spacing.sm),feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt (1)
175-175: InconsistentMifosTwoButtonRowpadding across sibling pages.This is the only page (among
ChargesPage,SettingsPage,PreviewPage, andDetailsPage) that appliesModifier.padding(top = KptTheme.spacing.sm)toMifosTwoButtonRow. The other three pages pass no modifier. If the top padding is intentional here (e.g., to visually separate the buttons from the content above), consider applying it consistently across all pages, or document why this page differs.feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInboxTasks/CheckerInboxTasksScreen.kt (1)
152-188: RedundantColumnwrapper around a singleRow.The
Columnat line 152 contains only a singleRowchild — no other composables are added. This adds an unnecessary layout level. Consider removing it and having theRowdirectly inside theCard.♻️ Proposed simplification
) { - Column { - Row( - modifier = Modifier - .padding(KptTheme.spacing.lg) - .fillMaxWidth(), - verticalAlignment = Alignment.CenterVertically, - ) { + Row( + modifier = Modifier + .padding(KptTheme.spacing.lg) + .fillMaxWidth(), + verticalAlignment = Alignment.CenterVertically, + ) { Icon( modifier = Modifier .size(DesignToken.sizes.iconMedium), painter = leadingIcon, contentDescription = null, tint = KptTheme.colorScheme.onSurfaceVariant, ) Text( modifier = Modifier .padding(start = KptTheme.spacing.md) .weight(1f), text = option, style = KptTheme.typography.bodyLarge, ) Card( colors = CardDefaults.cardColors( containerColor = KptTheme.colorScheme.surfaceContainerHighest, ), ) { Text( modifier = Modifier.padding( horizontal = DesignToken.spacing.medium, vertical = DesignToken.spacing.dp2, ), text = badge, style = KptTheme.typography.labelLarge, ) } - } } }feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInboxDialog/CheckerInboxTasksFilterDialog.kt (2)
253-256: Simplify.width()+.height()to.size()when both dimensions are identical.Both
widthandheightuse the sameDesignToken.spacing.extraLargeIncreasedvalue. Use.size()instead.♻️ Proposed simplification
modifier = Modifier - .width(DesignToken.spacing.extraLargeIncreased) - .height(DesignToken.spacing.extraLargeIncreased) + .size(DesignToken.spacing.extraLargeIncreased) .clickable { closeDialog.invoke() },
365-408: Extract duplicatedButtonColorsinto a local variable.Both buttons (lines 370–375 and 397–402) use identical
ButtonColorsconfigurations. Extract to a localvalto reduce duplication.♻️ Proposed refactor
Add a local variable before the
Row:val filterButtonColors = ButtonColors( containerColor = KptTheme.colorScheme.primary, contentColor = KptTheme.colorScheme.onPrimary, disabledContainerColor = KptTheme.colorScheme.primaryContainer, disabledContentColor = KptTheme.colorScheme.onPrimaryContainer, )Then reference
filterButtonColorsin both buttons:Button( onClick = { clearFilter.invoke() }, modifier = Modifier.height(DesignToken.spacing.dp40), - colors = ButtonColors( - containerColor = KptTheme.colorScheme.primary, - contentColor = KptTheme.colorScheme.onPrimary, - disabledContainerColor = KptTheme.colorScheme.primaryContainer, - disabledContentColor = KptTheme.colorScheme.onPrimaryContainer, - ), + colors = filterButtonColors, ) {(Same for the second button.)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
95-97: ResidualAppColorsandDesignTokenusage is acceptable for this migration scope.
AppColorsis used only for domain-specific loan status indicator colors (Lines 347–359), andDesignToken.sizesis used for component sizing tokens (icon and button sizes). These are reasonable to keep separate from KptTheme if they haven't been migrated yet. Consider tracking these for a follow-up migration pass.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSurveySubmit/SurveySubmitScreen.kt (1)
141-154: Inconsistent migration: hardcoded80.dpand40.dpremain while adjacent code was migrated.Line 141 uses
80.dpand Line 151 uses40.dpdirectly, while the card above was migrated to design tokens (e.g.,DesignToken.padding.dp40). Consider usingDesignToken.spacing.dp80andDesignToken.padding.dp40respectively for consistency.♻️ Proposed fix
- Spacer(modifier = Modifier.height(80.dp)) + Spacer(modifier = Modifier.height(DesignToken.spacing.dp80)) if (showButton) { Button( enabled = showButton, onClick = { submitSurvey.invoke() }, modifier = Modifier .fillMaxWidth() - .padding(horizontal = 40.dp), + .padding(horizontal = DesignToken.padding.dp40), ) {
...e/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetails/ClientDetailsScreen.kt
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt
Show resolved
Hide resolved
...s/feature/individualCollectionSheet/generateCollectionSheet/GenerateCollectionSheetScreen.kt
Outdated
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/component/RepaymentPeriodCard.kt
Outdated
Show resolved
Hide resolved
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
f5f7e4d to
7870eb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
225-229:⚠️ Potential issue | 🔴 CriticalBug:
ApproveAccountaction is created but never dispatched.The action object is constructed but not passed to
onAction, so tapping "Approve" on a pending recurring deposit is silently a no-op. Compare with theViewAccountbranch on Line 218 which correctly wraps the action inonAction(...).🐛 Proposed fix
is Actions.ApproveAccount -> { - RecurringDepositAccountAction.ApproveAccount( - recurringDeposit.accountNo ?: "", - ) + onAction( + RecurringDepositAccountAction.ApproveAccount( + recurringDeposit.accountNo ?: "", + ), + ) }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt (1)
199-209:⚠️ Potential issue | 🟡 MinorDead branch:
Feature.VIEW_DOCUMENTcan never be reached here.This
whenblock is inside theif (state.feature != Feature.VIEW_DOCUMENT)guard on line 190, so the branch at line 202 is unreachable. Theclient_identifier_titlestring will never be displayed.It also means the remaining two branches have inverted semantics —
documentKey == nullshows the "update" title and theelseshows the "add" title, which seems backwards.Suggested fix — remove dead branch and verify title logic
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) + else -> stringResource(Res.string.add_document_title) }, style = KptTheme.typography.titleMedium, )Please also verify the
documentKey == null→ "update" / else → "add" mapping is intentional; it reads like it should be the other way around.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccount/SavingsAccountScreen.kt (1)
398-408:⚠️ Potential issue | 🟡 MinorPre-existing layout issue:
AnimatedVisibilitycontent lacks aColumnwrapper.The
Spacer(Line 399) andMifosOutlinedTextField(Line 401) are siblings insideAnimatedVisibility, which uses aBoxinternally — so they overlap rather than stack. The secondAnimatedVisibilityblock (Line 434) correctly wraps its children in aColumn. This isn't introduced by this PR but is worth fixing.Suggested fix
) { + Column { Spacer(modifier = Modifier.height(KptTheme.spacing.md)) MifosOutlinedTextField( value = minimumRequiredBalance, onValueChange = { minimumRequiredBalance = it }, label = stringResource(Res.string.feature_savings_min_required_balance), error = null, keyboardType = KeyboardType.Number, ) + } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (1)
232-276:⚠️ Potential issue | 🟡 MinorRemove unused private
SelectionModeTopAppBaroverload.The private
SelectionModeTopAppBarfunction (lines 232–276) with signature(currentSelectedItems: List<ClientEntity>, syncClicked: () -> Unit, resetSelectionMode: () -> Unit)is dead code. The active call at lines 135–151 invokes the imported version fromcom.mifos.core.ui.componentswith a different signature (itemCount: Int,actionsparameter). The only call site for this private overload exists in the commented-out code at lines 152–156. Remove this unused function.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/addAddress/AddAddressScreen.kt (1)
351-384:⚠️ Potential issue | 🟡 MinorInconsistent button heights: Cancel uses
avatarMedium(48dp), Submit usesiconExtraLarge(36dp).Line 355 uses
DesignToken.sizes.avatarMediumfor the Cancel button height, while line 383 usesDesignToken.sizes.iconExtraLargefor the Submit button height. These are different values (48dp vs 36dp), causing the two side-by-side buttons to have mismatched heights.Both buttons should use the same height token. Change line 383 to use
DesignToken.sizes.avatarMediumfor consistency.Proposed fix
Button( modifier = Modifier .fillMaxWidth() .weight(0.4f) - .heightIn(DesignToken.sizes.iconExtraLarge), + .heightIn(DesignToken.sizes.avatarMedium),feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupDetails/GroupDetailsScreen.kt (1)
594-596:⚠️ Potential issue | 🟡 MinorMissed token migration: hardcoded
24.dpshould useDesignToken.sizes.iconMedium.In
MifosLoanAccountExpendableCard(Line 432), the equivalentIconButtonsize usesDesignToken.sizes.iconMedium, but here inMifosSavingsAccountExpendableCardit's still hardcoded to24.dp. This should be consistent.Proposed fix
IconButton( modifier = Modifier - .size(24.dp), + .size(DesignToken.sizes.iconMedium), onClick = { expendableState = !expendableState },feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccount/LoanAccountScreen.kt (1)
306-311:⚠️ Potential issue | 🟠 MajorBug: Disbursement date picker dismiss button closes the wrong picker.
Line 309 sets
showSubmissionDatePicker = falseinstead ofshowDisbursementDatePicker = false. This means dismissing the disbursement date picker won't actually close it (though the submission picker would be closed if somehow open).This appears to be a pre-existing bug, but since you're editing this file it's 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/clientLoanAccounts/ClientLoanAccountsScreen.kt (1)
170-201:⚠️ Potential issue | 🟡 MinorHardcoded "Not Available" strings break localization.
Multiple fields now use hardcoded English
"Not Available"instead ofstringResource(...). This is a l10n regression — these strings won't be translated for non-English locales.🌐 Proposed fix — extract to string resource
Consider using a string resource (e.g.,
stringResource(Res.string.client_savings_not_available)which already exists in this file's imports) instead of the hardcoded literal:accountNo = ( - loan.accountNo ?: "Not Available" + loan.accountNo ?: stringResource(Res.string.client_savings_not_available) ), -loanProduct = loan.productName ?: "Not Available", +loanProduct = loan.productName ?: stringResource(Res.string.client_savings_not_available), originalLoan = symbol + ( - (loan.originalLoan ?: "Not Available").toString() + (loan.originalLoan ?: stringResource(Res.string.client_savings_not_available)).toString() ),Apply the same pattern for
amountPaid(line 180),loanBalance(line 192), andtype(line 201).
🤖 Fix all issues with AI agents
In
`@feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInboxTasks/CheckerInboxTasksScreen.kt`:
- Around line 179-182: The modifier mixes semantic and dp-based spacing tokens
(DesignToken.spacing.medium with DesignToken.spacing.dp2); update the horizontal
padding to use the dp-based token to match vertical padding naming—replace the
horizontal value DesignToken.spacing.medium with DesignToken.spacing.dp12 in the
Modifier.padding call (look for the Modifier.padding usage in
CheckerInboxTasksScreen.kt referencing DesignToken.spacing).
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt`:
- Around line 56-61: The outer Column uses Modifier.fillMaxSize().padding(bottom
= KptTheme.spacing.sm) which may be an incorrect substitution for the prior
DesignToken.padding.large; verify and, if the original intent was a larger
bottom inset, change the token to KptTheme.spacing.md (or the appropriate larger
spacing token) in PreviewPage's outer Column modifier so the bottom padding
matches the previous value; confirm by inspecting KptTheme.spacing definitions
and update the token from spacing.sm to spacing.md where needed.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt`:
- Line 181: Rename the mistyped action ToggleFiler to ToggleFilter everywhere:
change the enum/sealed action ShareAccountsAction.ToggleFiler to
ShareAccountsAction.ToggleFilter, update the when/case branch in the
ShareAccountsViewModel that handles ToggleFiler to handle ToggleFilter (and call
the existing toggleFilter() handler), and update the UI reference in
ShareAccountsScreen where Modifier.onClick invokes
ShareAccountsAction.ToggleFiler to invoke ShareAccountsAction.ToggleFilter
instead so all symbols (ToggleFilter, toggleFilter(), and usages) are
consistent.
🧹 Nitpick comments (23)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
150-174: Remove commented-out search code.These ~25 lines of commented-out search bar/icon code add noise. If the feature is planned, track it in an issue rather than leaving dead code in the file.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
264-264: Nit: Remove migration breadcrumb comment.The
// TYPOGRAPHY: Mapped to KptThemecomment is a migration artifact that doesn't add value for future readers. Consider removing it for cleanliness — similar typography changes in other files don't carry this annotation.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt (1)
275-277: Redundant.map { it }— identity mapping creates an unnecessary list copy.
state.statusList.map { it }returns a new list identical to the original. Passstate.statusListdirectly.Suggested fix
- options = state.statusList.map { - it - }, + options = state.statusList,feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccount/SavingsAccountScreen.kt (1)
467-503: Minor inconsistency:DesignToken.spacing.dp44vsKptTheme.spacing.*.Line 470 references
DesignToken.spacing.dp44directly while Line 471 and all other spacings go throughKptTheme.spacing. Ifdp44is only available as a raw token inDesignTokenand not exposed viaKptTheme.spacing, this is fine — but worth confirming the intent is to use raw tokens for one-off sizes and semantic tokens (md) elsewhere.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt (2)
863-863: ResidualDesignToken.sizes.avatarMediumusage — consider migrating to KptTheme if a size token exists.
DesignToken.sizes.avatarMediumis still used for the buttonheightInon lines 863 and 891 while everything else in this composable now referencesKptTheme. IfKptThemeexposes an equivalent size token, migrating these two references would complete the transition for this file.Also applies to: 891-891
700-843: RepetitiveMifosOutlinedTextFieldconfiguration — consider extracting a helper.Each text field in
ClientInputTextFieldsrepeats the sameshape,textStyle,colors, and error-icon trailing-icon pattern. A small wrapper or shared defaults object would reduce ~150 lines of near-identical boilerplate.Example: extract shared defaults
// At file level or inside the composable `@Composable` private fun kptOutlinedTextFieldColors() = colors( focusedBorderColor = KptTheme.colorScheme.secondaryContainer, unfocusedBorderColor = KptTheme.colorScheme.secondaryContainer, errorBorderColor = KptTheme.colorScheme.error, ) `@Composable` private fun errorTrailingIcon(isError: Boolean): `@Composable` (() -> Unit)? = if (isError) { { Icon( imageVector = MifosIcons.Error, contentDescription = stringResource(Res.string.feature_client_error), tint = KptTheme.colorScheme.error, ) } } else nullThen each call site becomes:
MifosOutlinedTextField( ... shape = KptTheme.shapes.medium, textStyle = KptTheme.typography.bodyLarge, colors = kptOutlinedTextFieldColors(), config = MifosTextFieldConfig( isError = firstNameError != null, errorText = firstNameError, trailingIcon = errorTrailingIcon(firstNameError != null), ), )feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanCharge/LoanChargeScreen.kt (1)
242-256: Preview composable is inconsistent with other previews in the same module.
LoanAccountSummaryScreenin the same loan feature module wraps its preview inMifosTheme, but this preview does not. For consistency, wrap the preview content inMifosTheme { ... }to ensure design tokens are applied correctly at design time.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (1)
154-155: Remove leftover migration comment.Line 154 has a
// TYPOGRAPHY: Mapped to KptThemecomment that appears to be a migration tracking artifact. No other changed files carry these annotations.Suggested fix
- // TYPOGRAPHY: Mapped to KptTheme - style = KptTheme.typography.titleMedium, + style = KptTheme.typography.titleMedium,feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (1)
292-293: Remove migration tracking comment.This inline comment (
// SPACING: Replaced DesignToken.padding.medium with KptTheme.spacing.md) is a migration artifact and shouldn't be committed. The git history already captures what was replaced.Proposed fix
- // SPACING: Replaced DesignToken.padding.medium with KptTheme.spacing.md Spacer(Modifier.height(KptTheme.spacing.md))feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (2)
152-156: Remove commented-out code.This old
SelectionModeTopAppBarinvocation is dead code. It adds noise and there's already a replacement above it (lines 135–151). Delete the block to keep the file clean.🧹 Proposed cleanup
-// SelectionModeTopAppBar( -// currentSelectedItems = selectedItems.selectedItems.value, -// syncClicked = { sync.value = true }, -// resetSelectionMode = resetSelectionMode, -// )
388-393: Spacing token used for icon sizing.
KptTheme.spacing.lgis a spacing/padding token being used as an iconsize. This works at runtime but is semantically misleading. Prefer a size token (e.g.,DesignToken.sizes.*or a dedicated icon-size token) so intent is clear and future spacing-only changes don't inadvertently resize icons.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/addAddress/AddAddressScreen.kt (1)
262-268: Consider usingstyleparameter instead of extracting individual typography properties.Lines 265–267 (and similarly 372–374, 397–399) manually extract
fontSize,letterSpacing, andlineHeightfromKptTheme.typography.labelLarge. Using thestyleparameter onTextis more concise and automatically picks up all properties, with overrides applied viafontWeight.Example refactor
Text( text = stringResource(Res.string.feature_client_add_address), - fontWeight = FontWeight.SemiBold, - fontSize = KptTheme.typography.labelLarge.fontSize, - letterSpacing = KptTheme.typography.labelLarge.letterSpacing, - color = KptTheme.colorScheme.onSurface, + style = KptTheme.typography.labelLarge.copy( + fontWeight = FontWeight.SemiBold, + color = KptTheme.colorScheme.onSurface, + ), )feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/component/RepaymentPeriodCard.kt (1)
132-139: Inconsistent color specification for paid/due text.The status label (line 132–134) embeds the color inside
style.copy(color = ...), while the amount text (lines 138–139) passes color as a separateTextparameter. Both approaches work, but mixing them in adjacent composables is slightly inconsistent. Consider picking one approach for both.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
586-586: Divider thickness doubled from0.5.dptoDesignToken.spacing.dp1(1.dp).Minor visual change — the data table row dividers are now twice as thick as before. If the thinner hairline divider was intentional, consider adding a
dp0_5token. Otherwise this is fine as standardization on the smallest available token.feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/generateCollectionSheet/GenerateCollectionSheetScreen.kt (1)
363-364:heightIn(DesignToken.spacing.dp44)sets only a minimum height — verify this is intentional.
Modifier.heightIn(DesignToken.spacing.dp44)passes the value as theminparameter (with nomax), so the button can grow unbounded. If a fixed height of 44dp is intended, use.height(DesignToken.spacing.dp44)instead. If minimum height is the goal (allowing content to expand), this is fine. The same pattern appears on lines 422 and 502.feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInboxTasks/CheckerInboxTasksScreen.kt (1)
152-188: Redundant wrappingColumnaround a singleRow.The
Columnat line 152 wraps only a singleRowchild, adding an unnecessary layout layer. This doesn't affect functionality but adds a layout pass.♻️ Proposed simplification
) { - Column { - Row( - modifier = Modifier - .padding(KptTheme.spacing.lg) - .fillMaxWidth(), - verticalAlignment = Alignment.CenterVertically, - ) { + Row( + modifier = Modifier + .padding(KptTheme.spacing.lg) + .fillMaxWidth(), + verticalAlignment = Alignment.CenterVertically, + ) { // ... icon, text, badge card ... - } } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (1)
395-395: Raw1.5.dpthickness mixed with theme tokens.The rest of the file consistently uses
KptTheme/DesignTokentokens, but this divider thickness is a hardcoded dp value. Consider using a token if one exists, or at minimum note this as a future cleanup.feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt (1)
254-278: EmptySavingsCard theming looks good.Border, padding, and typography properly use KptTheme/DesignToken tokens. Note that the card text content (
"No Item Found","Click Here To View Filled State.") appears to be pre-existing hardcoded strings — consider extracting them to string resources for i18n in a follow-up.feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt (1)
59-85: Remove commented-out code.Lines 60–61 contain the old
title/onBackPressedparameters as comments. Since the replacementTopAppBaris already in place, these dead comments add noise.♻️ Suggested cleanup
MifosScaffold( -// title = stringResource(Res.string.feature_about), -// onBackPressed = onBackPressed, snackbarHostState = remember { SnackbarHostState() }, topBar = {feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/components/ProfileCard.kt (1)
107-112: Spacing token used for icon sizing.Line 110 uses
KptTheme.spacing.mdfor the chevron iconsize, but spacing tokens are semantically meant for padding/margins. Other icons in this file correctly useDesignToken.sizes.iconSmall(line 136). Consider using an appropriate size token (e.g.,DesignToken.sizes.iconSmall) for consistency.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt (1)
966-972:DesignToken.spacing.dp2used for border width.
DesignToken.spacing.dp2is semantically a spacing token, but here it's used as a border width. This works but is a slight semantic mismatch. Consider whether a dedicated border-width token (e.g., inDesignToken.sizesor abordersgroup) would be clearer long-term.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (1)
225-270: HardcodedColor.Red.copy(alpha = .5f)remains in the header background.Line 229 still uses a hardcoded red color for the header background while the rest of the file has been migrated to
KptTheme.colorSchemetokens. Consider usingKptTheme.colorScheme.errorContainer(or a similar semantic token) for consistency with the theme migration.♻️ Suggested change
Box( modifier = Modifier - .background(Color.Red.copy(alpha = .5f)) + .background(KptTheme.colorScheme.errorContainer) .fillMaxWidth(), ) {feature/document/src/commonMain/kotlin/com/mifos/feature/document/documentDialog/DocumentDialogScreen.kt (1)
252-279: Dialog container styling and icon sizing migrated correctly.The shape, background, padding, typography, and color references all properly use
KptThemetokens. The icon usesDesignToken.spacing.dp30for both dimensions — consider using a size token (e.g., fromDesignToken.sizes) instead of a spacing token for element dimensions, as it would be more semantically appropriate.
...ain/kotlin/com/mifos/feature/checker/inbox/task/checkerInboxTasks/CheckerInboxTasksScreen.kt
Show resolved
Hide resolved
...lient/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt
Outdated
Show resolved
Hide resolved
|
|
||
| Spacer(modifier = Modifier.width(KptTheme.spacing.sm)) | ||
| Icon( | ||
| modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.ToggleFiler) }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check all references to ToggleFiler across the codebase
rg -n 'ToggleFiler' --type=ktRepository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Also check if ToggleFilter exists
rg -n 'ToggleFilter' --type=ktRepository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Find the ShareAccountsAction definition to see all action names
fd 'ShareAccountsAction' --type=ktRepository: openMF/android-client
Length of output: 234
🏁 Script executed:
# Search for ToggleFiler references using pattern matching
rg 'ToggleFiler' -G '*.kt'Repository: openMF/android-client
Length of output: 469
🏁 Script executed:
# Search for ToggleFilter references
rg 'ToggleFilter' -G '*.kt'Repository: openMF/android-client
Length of output: 469
🏁 Script executed:
# Find the ShareAccountsAction definition file
fd 'ShareAccountsAction'Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Read the specific file mentioned in the review to check the context
cat -n feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt | sed -n '170,185p'Repository: openMF/android-client
Length of output: 799
🏁 Script executed:
# Find files containing ShareAccountsAction definition
rg 'sealed class ShareAccountsAction|object ShareAccountsAction|enum class ShareAccountsAction' --max-count=5Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Search in the shareAccounts directory for the action definition
find feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts -name '*.kt' -type fRepository: openMF/android-client
Length of output: 363
🏁 Script executed:
# Also try a broader search for all mentions of ToggleFiler or ToggleFilter
rg 'Toggle(F|f)il(e|)r'Repository: openMF/android-client
Length of output: 573
🏁 Script executed:
# Find the ShareAccountsAction definition section
rg -A 15 'sealed class ShareAccountsAction|data object ToggleFiler' feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsViewModel.ktRepository: openMF/android-client
Length of output: 356
🏁 Script executed:
# Show more context around action definitions
cat -n feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsViewModel.kt | rg -A 20 -B 5 'ShareAccountsAction'Repository: openMF/android-client
Length of output: 2202
Typo: ToggleFiler → ToggleFilter across action definition and all references.
ToggleFiler should be ToggleFilter — the action handler function is named toggleFilter(), the icon resource is Res.drawable.filter, and this aligns with the naming pattern used elsewhere (e.g., ToggleSearchBar). Update:
- Action definition in
ShareAccountsViewModel.kt(line 117) - Handler case in
ShareAccountsViewModel.kt(line 32) - Usage in
ShareAccountsScreen.kt(line 181)
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt`
at line 181, Rename the mistyped action ToggleFiler to ToggleFilter everywhere:
change the enum/sealed action ShareAccountsAction.ToggleFiler to
ShareAccountsAction.ToggleFilter, update the when/case branch in the
ShareAccountsViewModel that handles ToggleFiler to handle ToggleFilter (and call
the existing toggleFilter() handler), and update the UI reference in
ShareAccountsScreen where Modifier.onClick invokes
ShareAccountsAction.ToggleFiler to invoke ShareAccountsAction.ToggleFilter
instead so all symbols (ToggleFilter, toggleFilter(), and usages) are
consistent.
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
I have reviewed 13/130 files till now. Please follow the recommendations for all the files.
| // title = stringResource(Res.string.feature_about), | ||
| // onBackPressed = onBackPressed, | ||
| snackbarHostState = remember { SnackbarHostState() }, |
There was a problem hiding this comment.
Can you tell us the reason for commenting out these lines?
There was a problem hiding this comment.
it was like this before. check L.65-67 it already defines title. also i guess this commented out line got into my PR during the conflict resolution.
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(6.dp)) | ||
| Spacer(modifier = Modifier.height(KptTheme.spacing.sm)) |
There was a problem hiding this comment.
I think KptTheme.spacing.sm is 8.dp and not 6.dp. Please check and add 6.dp in DesignToken and use it here.
There was a problem hiding this comment.
okay. i made certain adjustments to keep uniformity across the screens. a lot of files were not following that thus I adjusted the spacings/padding a bit. if you say i can add them to DesignToken and use previous token values
There was a problem hiding this comment.
Since this PR is for migration, let’s avoid UI changes for now. Please keep the existing values and move them to DesignToken. We can raise separate tickets to address UI improvements later.
| .padding(KptTheme.spacing.md), | ||
| text = it, | ||
| style = MaterialTheme.typography.titleMedium, | ||
| style = KptTheme.typography.titleLarge, |
| .padding(KptTheme.spacing.md), | ||
| text = stringResource(Res.string.feature_center_summary_info), | ||
| style = MaterialTheme.typography.titleMedium, | ||
| style = KptTheme.typography.titleLarge, |
| .padding(KptTheme.spacing.sm), | ||
| text = group.name.toString(), | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| style = KptTheme.typography.bodyLarge, |
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(12.dp)) | ||
| Spacer(modifier = Modifier.height(KptTheme.spacing.md)) |
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(12.dp)) | ||
| Spacer(modifier = Modifier.height(KptTheme.spacing.md)) |
| } | ||
|
|
||
| Spacer(modifier = Modifier.width(10.dp)) | ||
| Spacer(modifier = Modifier.width(KptTheme.spacing.sm)) |
| modifier = Modifier.padding(8.dp), | ||
| elevation = CardDefaults.elevatedCardElevation(4.dp), | ||
| modifier = Modifier.padding(KptTheme.spacing.sm), | ||
| elevation = CardDefaults.elevatedCardElevation(KptTheme.elevation.level1), |
There was a problem hiding this comment.
Please don't round off to nearest value. If the exact size is not present, add it to the DesignToken, later on it will be incorporated in the KptTheme appropriately.
| contentPadding = PaddingValues(KptTheme.spacing.md), | ||
| verticalArrangement = Arrangement.spacedBy(KptTheme.spacing.md), |
There was a problem hiding this comment.
Same here, please review.
Here is the mapping table for reference:
openMF/mifos-mobile#3013 (comment)
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
7870eb9 to
41ecdad
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/groupLoanAccount/GroupLoanAccountScreen.kt (1)
295-299:⚠️ Potential issue | 🟠 MajorBug: Dismiss button in disbursement date picker closes the wrong dialog.
Line 298 sets
showSubmissionDatePicker = falseinstead ofshowDisbursementDatePicker = false. If the user opens the disbursement date picker and clicks "Cancel", the dialog won't dismiss (sinceshowDisbursementDatePickerremainstrue).This appears to be a pre-existing copy-paste bug, but since this file is being touched, it's worth fixing now.
🐛 Proposed fix
dismissButton = { TextButton( onClick = { - showSubmissionDatePicker = false + showDisbursementDatePicker = false }, ) { Text(stringResource(Res.string.feature_loan_cancel)) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/groupLoanAccount/GroupLoanAccountScreen.kt` around lines 295 - 299, The dismiss button in the disbursement date picker is toggling the wrong state variable; change the onClick handler in the disbursement dialog so it sets showDisbursementDatePicker = false (not showSubmissionDatePicker = false). Locate the dismissButton lambda for the disbursement DatePicker in GroupLoanAccountScreen and update the onClick to close the disbursement dialog by referencing showDisbursementDatePicker.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/DetailsPage.kt (1)
260-260:⚠️ Potential issue | 🟡 MinorIncomplete migration:
DesignToken.padding.mediumnot replaced withKptTheme.spacing.md.Line 260 is the only remaining
DesignTokenusage in this file. Every other spacer has been migrated toKptTheme.spacing.md. This also keeps theDesignTokenimport on line 52 alive solely for this one call.Proposed fix
- Spacer(Modifier.height(DesignToken.padding.medium)) + Spacer(Modifier.height(KptTheme.spacing.md))And remove the now-unused import:
-import com.mifos.core.designsystem.theme.DesignToken🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/DetailsPage.kt` at line 260, Replace the remaining use of DesignToken.padding.medium inside the DetailsPage composable (the Spacer call) with KptTheme.spacing.md and remove the now-unused DesignToken import; locate the Spacer(...) invocation in DetailsPage.kt and update its Modifier.height argument to use KptTheme.spacing.md, then delete the DesignToken import from the file imports to avoid an unused import.feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupDetails/GroupDetailsScreen.kt (1)
594-596:⚠️ Potential issue | 🟡 MinorInconsistent hardcoded
24.dp— should useDesignToken.sizes.iconMedium.The loan expandable card (Line 432) was migrated to
DesignToken.sizes.iconMedium, but this savings counterpart still uses a hardcoded24.dp. This should be aligned for consistency.Proposed fix
IconButton( modifier = Modifier - .size(24.dp), + .size(DesignToken.sizes.iconMedium), onClick = { expendableState = !expendableState },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupDetails/GroupDetailsScreen.kt` around lines 594 - 596, Replace the hardcoded IconButton size of 24.dp with the design token constant to maintain consistency: locate the IconButton instantiation in GroupDetailsScreen (the one using Modifier.size(24.dp)) and change it to use DesignToken.sizes.iconMedium for its size; ensure any imports remain correct and that the same symbol (DesignToken.sizes.iconMedium) is used as in the loan expandable card migration.feature/data-table/src/commonMain/kotlin/com/mifos/feature/dataTable/dataTableRowDialog/DataTableRowDialogScreen.kt (1)
105-111:⚠️ Potential issue | 🟠 MajorPre-existing: side effects run directly during composition without
LaunchedEffect.
scope.launch { snackbarHostState.showSnackbar(...) }andonSuccess.invoke()are executed directly in the composition body (Lines 107–110). This means they fire on every recomposition when the state isDataTableEntrySuccessfully, not just once. This should be wrapped in aLaunchedEffectkeyed on the state to ensure it fires exactly once.Not introduced by this PR, but worth addressing.
Suggested fix
is DataTableRowDialogUiState.DataTableEntrySuccessfully -> { val message = stringResource(Res.string.feature_data_table_added_data_table_successfully) - scope.launch { - snackbarHostState.showSnackbar(message) + LaunchedEffect(Unit) { + snackbarHostState.showSnackbar(message) } onSuccess.invoke() }Note:
onSuccess.invoke()also needs to be moved inside aLaunchedEffector handled via a state-driven callback to avoid repeated invocations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/data-table/src/commonMain/kotlin/com/mifos/feature/dataTable/dataTableRowDialog/DataTableRowDialogScreen.kt` around lines 105 - 111, The code currently launches side effects directly during composition when encountering DataTableRowDialogUiState.DataTableEntrySuccessfully (using scope.launch { snackbarHostState.showSnackbar(...) } and onSuccess.invoke()), causing them to run on every recomposition; move these into a LaunchedEffect keyed on the ui state (e.g., LaunchedEffect(uiState) or LaunchedEffect(DataTableRowDialogUiState.DataTableEntrySuccessfully)) and inside that block call snackbarHostState.showSnackbar(...) and onSuccess.invoke() so the snackbar and callback run exactly once per state transition instead of on every recomposition.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 199 guards this block with
state.feature != Feature.VIEW_DOCUMENT, so thewhenbranch at line 202 (state.feature == Feature.VIEW_DOCUMENT) can never be true. The string at line 202 is unreachable.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, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt` around lines 199 - 209, The when branch checking state.feature == Feature.VIEW_DOCUMENT inside the Text composable of ClientIdentitiesAddUpdateScreen is unreachable because the whole block is already guarded by if (state.feature != Feature.VIEW_DOCUMENT); remove that dead branch and simplify the Text selection to only depend on state.documentKey (e.g., replace the when with a simple if (state.documentKey == null) -> client_update_document_title else -> add_document_title) or, if the VIEW_DOCUMENT case was intended, move the outer guard logic to allow handling Feature.VIEW_DOCUMENT and keep the when as-is—update the Text logic in ClientIdentitiesAddUpdateScreen accordingly.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccount/LoanAccountScreen.kt (1)
306-312:⚠️ Potential issue | 🟠 MajorPre-existing bug: dismiss button closes wrong date picker.
The dismiss button for the disbursement date picker dialog sets
showSubmissionDatePicker = false(line 309) instead ofshowDisbursementDatePicker = false. This means canceling the disbursement date picker won't actually close it.Proposed fix
dismissButton = { TextButton( onClick = { - showSubmissionDatePicker = false + showDisbursementDatePicker = false }, ) { Text(stringResource(Res.string.feature_loan_cancel)) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccount/LoanAccountScreen.kt` around lines 306 - 312, The dismissButton lambda in LoanAccountScreen.kt is closing the wrong dialog by setting showSubmissionDatePicker = false; change the assignment inside the dismissButton onClick to set showDisbursementDatePicker = false instead so the disbursement date picker actually closes (update the dismissButton block that currently references showSubmissionDatePicker).feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/syncSurvey/SyncSurveysDialog.kt (1)
441-450:⚠️ Potential issue | 🟡 MinorPreview should be wrapped in
KptThemeto supply required theme tokens.The composable now depends on
KptTheme.colorScheme,KptTheme.shapes, andKptTheme.spacing. Without wrapping inKptTheme { ... }, the preview will either crash or render with incorrect defaults.Proposed 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 = {}) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/syncSurvey/SyncSurveysDialog.kt` around lines 441 - 450, Wrap the preview contents in KptTheme so the composable has the required theme tokens; update the SyncSurveysDialogPreview composable to call KptTheme { ... } and place the Column and all SyncSurveysDialog(...) calls inside that lambda so KptTheme.colorScheme, KptTheme.shapes and KptTheme.spacing are available during preview rendering.feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/savedIndividualCollectionSheet/SavedIndividualCollectionSheetCompose.kt (1)
52-56:⚠️ Potential issue | 🟡 MinorAdd
KptThemewrapper to preview for correct theme context.The composable uses
KptTheme.colorScheme.surface(line 35) andKptTheme.typography.headlineSmall(line 46), which are@Composableproperties that requireCompositionLocalvalues to be provided. The preview should be wrapped inKptTheme { … }to ensure proper theme context, consistent with the pattern used in other previews likeBounceAnimation.Diff
`@DevicePreview` `@Composable` private fun SavedIndividualCollectionSheetComposePreview() { - SavedIndividualCollectionSheetCompose() + KptTheme { + SavedIndividualCollectionSheetCompose() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/savedIndividualCollectionSheet/SavedIndividualCollectionSheetCompose.kt` around lines 52 - 56, The preview SavedIndividualCollectionSheetComposePreview must be wrapped in the app theme so KptTheme CompositionLocals are provided; update the SavedIndividualCollectionSheetComposePreview function to call KptTheme { SavedIndividualCollectionSheetCompose() } so KptTheme.colorScheme.surface and KptTheme.typography.headlineSmall resolve correctly when rendering the preview.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
116-189:⚠️ Potential issue | 🟠 MajorThe button section may be pushed off-screen due to
fillMaxSize()on the scrollable Column.Line 120's Column uses
Modifier.fillMaxSize()which will consume all available vertical space in the parent Column, leaving zero height for the button Column at Line 154. The "Delete Photo" and "Upload New Photo" buttons would not be visible.Replace
fillMaxSize()withweight(1f)on the scrollable content Column so it fills remaining space after the button Column is measured.🐛 Proposed fix
Column( modifier = Modifier - .fillMaxSize() + .fillMaxWidth() + .weight(1f) .verticalScroll(rememberScrollState()) .padding(horizontal = KptTheme.spacing.md), horizontalAlignment = Alignment.CenterHorizontally,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt` around lines 116 - 189, The scrollable content Column (the one using Modifier.fillMaxSize().verticalScroll(...) and displaying state.name, profile image, etc.) is consuming all vertical space and pushing the button Column (containing MifosOutlinedButton and MifosTextButton) off-screen; replace Modifier.fillMaxSize() on that scrollable Column with Modifier.weight(1f) (preserving the verticalScroll and padding) so it takes the remaining space and allows the button Column to be measured and visible. Locate the Column that calls verticalScroll(rememberScrollState()) and adjust its modifier accordingly; the button block to verify visibility includes MifosOutlinedButton and MifosTextButton.
| androidx.compose.material3.adaptive:adaptive-android:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-layout-android:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-layout:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-navigation-android:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-navigation:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive:1.1.0 | ||
| androidx.compose.material3:material3-adaptive-navigation-suite-android:1.3.2 | ||
| androidx.compose.material3:material3-adaptive-navigation-suite:1.3.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
compose-bom 2025.01.00 androidx compose material3 adaptive versions
💡 Result:
For Compose BOM androidx.compose:compose-bom:2025.01.00 (Jan 15, 2025), the Compose Material3 Adaptive artifacts resolve to version 1.0.0:
androidx.compose.material3.adaptive:adaptive:1.0.0androidx.compose.material3.adaptive:adaptive-layout:1.0.0androidx.compose.material3.adaptive:adaptive-navigation:1.0.0[1]
(At that time, newer Adaptive versions like 1.1.0 were not yet released—1.1.0 came later on Mar 12, 2025.) [2]
Sources:
[1] BOM → library mapping table (Android Developers) (developer.android.com)
[2] Compose Material3 Adaptive release notes (Android Developers) (developer.android.com)
Citations:
- 1: https://developer.android.com/develop/ui/compose/bom/bom-mapping
- 2: https://developer.android.com/jetpack/androidx/releases/compose-material3-adaptive?utm_source=openai
🌐 Web query:
androidx.compose.material3.adaptive 1.1.0 material3 1.3.2 compatibility
💡 Result:
androidx.compose.material3.adaptive:adaptive:1.1.0 and androidx.compose.material3:material3:1.3.2 are not a “known-good” matched set, because they were published against different Compose core versions:
- adaptive 1.1.0 pulls Compose Foundation 1.6.5 (and
ui-geometry 1.6.5). [1] (Release date: Mar 12, 2025. [2]) - material3 1.3.2 pulls Compose Runtime/Foundation 1.7.0 (and
ui/ui-text 1.6.0). [3] (Release date: Apr 9, 2025. [4])
Gradle will try to resolve these to a single set of Compose artifacts, but mixing like this can lead to runtime/linkage issues (e.g., NoSuchMethodError) because Compose libraries are intended to be used with aligned versions.
Recommendation: use the Compose BOM (no versions on individual Compose artifacts) so material3 and material3.adaptive resolve to compatible versions, or upgrade material3.adaptive to a newer release and keep everything aligned via BOM. [5]
Sources: [1] [2] [3] [4] [5]
🌐 Web query:
material3-adaptive-navigation-suite 1.3.2 compose requirements
💡 Result:
androidx.compose.material3:material3-adaptive-navigation-suite:1.3.2 (released Apr 9, 2025) is part of Compose Material3 1.3.2. [1][2]
What you need (Compose “requirements”)
- Use matching Compose libraries (recommended: Compose BOM) so Material3 1.3.2 lines up with the rest of Compose. The BOM released the same day is
androidx.compose:compose-bom:2025.04.00(Apr 9, 2025). [1][3] - Transitive deps you’ll get include:
androidx.compose.material3:material3:1.3.2androidx.compose.material3.adaptive:adaptive:1.0.0[4]
Gradle (recommended)
dependencies {
implementation(platform("androidx.compose:compose-bom:2025.04.00"))
implementation("androidx.compose.material3:material3-adaptive-navigation-suite")
}BOM usage is the standard way to keep Compose artifacts compatible. [1][5]
Compose compiler / Kotlin
The Compose Compiler is not in the BOM; pick it via the Compose ↔ Kotlin compatibility map (or use the Compose Compiler Gradle plugin if you’re on Kotlin 2.0+). [5][6]
Sources:
[1] AndroidX stable channel (Apr 9, 2025: Compose Material3 1.3.2) — developer.android.com
[2] MvnRepository artifact page (1.3.2 release date) — mvnrepository.com
[3] MvnRepository Compose BOM versions (2025.04.00 date) — mvnrepository.com
[4] MvnRepository dependency list for 1.3.2 — mvnrepository.com
[5] “Use a Bill of Materials” (BOM + compiler note) — developer.android.com
[6] Compose to Kotlin Compatibility Map — developer.android.com
Remove adaptive 1.1.0 or upgrade to compose-bom 2025.04.00—current version set causes Compose library misalignment.
The artifacts at lines 23-30 use adaptive 1.1.0 and material3-adaptive-navigation-suite 1.3.2 (both released Apr 9, 2025), but compose-bom 2025.01.00 only includes adaptive 1.0.0. More critically, adaptive 1.1.0 depends on Compose Foundation 1.6.5, while material3 1.3.2 requires Foundation 1.7.0—this version mismatch can cause runtime failures (e.g., NoSuchMethodError).
Either downgrade adaptive to 1.0.0 to match the BOM, or upgrade the entire BOM to 2025.04.00 (released Apr 9, 2025) to align all Compose libraries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmp-android/dependencies/demoDebugRuntimeClasspath.txt` around lines 23 - 30,
The Compose dependency set is misaligned: the adaptive artifacts
(androidx.compose.material3.adaptive:adaptive:1.1.0 and related adaptive-*
entries) conflict with
androidx.compose.material3:material3-adaptive-navigation-suite:1.3.2 because
your compose-bom (2025.01.00) only includes adaptive 1.0.0 and 1.1.0 pulls in
Foundation 1.6.5 while material3 1.3.2 needs Foundation 1.7.0; fix by either
downgrading all adaptive artifacts
(androidx.compose.material3.adaptive:adaptive, adaptive-android,
adaptive-layout, adaptive-navigation, adaptive-layout-android,
adaptive-navigation-android) to 1.0.0 to match the current BOM, or upgrade your
compose-bom to 2025.04.00 so the adaptive 1.1.0 and
material3-adaptive-navigation-suite:1.3.2 are aligned across Foundation
versions.
| org.jetbrains.androidx.navigation:navigation-runtime:2.9.0-beta03 | ||
| org.jetbrains.androidx.savedstate:savedstate-compose:1.3.4 | ||
| org.jetbrains.androidx.savedstate:savedstate:1.3.4 | ||
| org.jetbrains.androidx.window:window-core:1.3.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
org.jetbrains.androidx.window window-core 1.3.1 Compose Multiplatform
💡 Result:
org.jetbrains.androidx.window:window-core:1.3.1 is JetBrains’ Kotlin Multiplatform–published variant of Jetpack WindowManager Core (used in Compose Multiplatform/common code for things like WindowSizeClass). Version 1.3.1 was published Nov 12, 2024 and a newer line exists (for example 1.5.1). [1]
How to use in Compose Multiplatform (Gradle Kotlin DSL)
repositories {
mavenCentral()
// If Gradle can’t resolve org.jetbrains.androidx.* in your setup, add:
maven("https://maven.pkg.jetbrains.space/public/p/compose/dev")
}
kotlin {
sourceSets {
commonMain.dependencies {
implementation("org.jetbrains.androidx.window:window-core:1.3.1")
}
}
}JetBrains documents the Compose dev repository above for accessing JetBrains-published Compose/KMP artifacts. [2]
Version note
Jetpack WindowManager stable has moved well past 1.3.x (for example 1.5.1 is stable as of Nov 19, 2025). [3] Compose Multiplatform release notes also show newer recommended org.jetbrains.androidx.window:window-core versions (for example 1.5.1 in recent releases), so if you’re on a newer Compose Multiplatform version, prefer the version it recommends. [4]
Sources: [1] [2] [3] [4]
🌐 Web query:
androidx.window 1.3.0 vs jetbrains androidx.window compatibility conflict
💡 Result:
This conflict usually happens because you’re pulling two different “WindowManager” lines into the same build:
- Google Jetpack artifacts:
androidx.window:window-*(e.g., 1.3.0, released May 29, 2024). (developer.android.com) - JetBrains republished KMP artifacts:
org.jetbrains.androidx.window:window-core(used by Compose Multiplatform and some JetBrains “androidx.*” ports; e.g., CMP 1.8.2 mentionsorg.jetbrains.androidx.window:window-core:1.4.0-alpha07). (jetbrains.com)
When both end up on the same classpath (common causes: adding androidx.window:* directly, while some CMP library pulls org.jetbrains.androidx.window:* transitively), you can get dependency resolution/binary mismatch or duplicate/overlapping classes.
What to do (pick one “family” per target)
A) Android-only (recommended if you don’t need the JetBrains KMP port):
- Use only
androidx.window:window-*(1.3.0 or newer), and excludeorg.jetbrains.androidx.windowfrom any dependency that brings it in.
B) Compose Multiplatform / KMP common code using JetBrains ports:
- Use only
org.jetbrains.androidx.window:*in common code, and don’t addandroidx.window:*directly (unless you’re doing a deliberate Android/JVM-only substitution).
Known practical workaround (from JetBrains team guidance)
If a JetBrains library drags in org.jetbrains.androidx.window but you want the Google AndroidX implementation for JVM/Android, exclude the JetBrains group and add the AndroidX dependency explicitly (example shown for adaptive; same idea applies generally): (slack-chats.kotlinlang.org)
implementation("org.jetbrains.compose.material3.adaptive:adaptive:<ver>") {
exclude(group = "org.jetbrains.androidx.window")
}
implementation("androidx.window:window-core-jvm:1.4.0")If you paste your Gradle dependency tree section that contains both androidx.window and org.jetbrains.androidx.window (e.g., from ./gradlew :module:dependencies), I can point to the exact transitive edge and the smallest exclude/substitution to apply.
Citations:
- 1: https://developer.android.com/jetpack/androidx/releases/window?utm_source=openai
- 2: https://www.jetbrains.com/help/kotlin-multiplatform-dev/whats-new-compose-180.html?utm_source=openai
- 3: https://slack-chats.kotlinlang.org/t/28933788/when-using-compose-1-9-0-alpha02-i-get-the-following-error-d?utm_source=openai
🌐 Web query:
JetBrains Compose Multiplatform androidx.window version requirements
💡 Result:
-
For Compose Multiplatform (shared/common code), don’t depend on
androidx.window:*directly. Use JetBrains’ repackaged KMP artifact:org.jetbrains.androidx.window:window-core, and match its version to your Compose Multiplatform plugin/version.- Example: Compose Multiplatform 1.10.1 →
org.jetbrains.androidx.window:window-core:1.5.1. [1]
- Example: Compose Multiplatform 1.10.1 →
-
On Android-only code, the upstream Jetpack WindowManager stable line is currently
androidx.window:*:1.5.1(released Nov 19, 2025). [2] -
Older CMP versions pin older window-core (so align accordingly): e.g. Compose Multiplatform 1.9.1 →
org.jetbrains.androidx.window:window-core:1.4.0. [3]
Gradle (KMP commonMain)
commonMain.dependencies {
implementation("org.jetbrains.androidx.window:window-core:<match your CMP version>")
}🏁 Script executed:
cat cmp-android/dependencies/demoDebugRuntimeClasspath.txtRepository: openMF/android-client
Length of output: 20876
Remove artifact family mixing and update window-core to match Compose Multiplatform 1.9.0.
Both androidx.window (Google Jetpack) and org.jetbrains.androidx.window (JetBrains KMP) are present in the classpath, which causes dependency resolution conflicts and duplicate classes. For Compose Multiplatform builds, use only org.jetbrains.androidx.window:window-core and exclude the Google artifacts. Additionally, org.jetbrains.androidx.window:window-core:1.3.1 is outdated for CMP 1.9.0; update to 1.4.0 or newer per JetBrains Compose Multiplatform version alignment guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmp-android/dependencies/demoDebugRuntimeClasspath.txt` at line 386, The
classpath contains mixed artifact families (Google Jetpack androidx.window and
JetBrains org.jetbrains.androidx.window) and an outdated JetBrains window-core
version; remove any Google artifacts (androidx.window:window-*) from the
dependency list/resolution and ensure only
org.jetbrains.androidx.window:window-core is used, updating its version from
1.3.1 to 1.4.0 (or newer) to align with Compose Multiplatform 1.9.0; if needed,
add an explicit enforced/platform/constraints entry to force
org.jetbrains.androidx.window:window-core:1.4.0 and/or add excludes for
androidx.window artifacts where transitive pulls occur.
| org.jetbrains.compose.material3.adaptive:adaptive-layout:1.1.2 | ||
| org.jetbrains.compose.material3.adaptive:adaptive-navigation:1.1.2 | ||
| org.jetbrains.compose.material3.adaptive:adaptive:1.1.2 | ||
| org.jetbrains.compose.material3:material3-adaptive-navigation-suite:1.8.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JetBrains Compose 1.9.0 material3.adaptive 1.1.2 compatibility
💡 Result:
JetBrains Compose Multiplatform 1.9.0 can usually be used with org.jetbrains.compose.material3.adaptive:adaptive:1.1.2, but 1.1.2 is not aligned with the 1.9.x Compose line:
org.jetbrains.compose.material3.adaptive:adaptive:1.1.2(Jun 17, 2025) is built as a thin wrapper that brings in Jetpackandroidx.compose.material3.adaptive:adaptive:1.1.0and also has runtime deps on Compose Multiplatform artifacts at ~1.8.2(foundation, ui-geometry, etc.). So when used in a 1.9.0 project, Gradle will typically resolve those to your newer Compose versions, but this is a version mix you should treat as “works unless you hit an ABI/API edge”. [1]- For the 1.9.x Compose Multiplatform line, JetBrains’ docs indicate Material3 Adaptive should be
org.jetbrains.compose.material3.adaptive:adaptive*:1.2.0, based on Jetpack Material3 Adaptive 1.2.0. That’s the safer “known-aligned” choice. [2] - Jetpack Material3 Adaptive releases show 1.1.0 (Mar 12, 2025) and 1.2.0 (Oct 22, 2025); there is no Jetpack 1.1.2—the “1.1.2” you’re referring to is the JetBrains artifact version. [3]
Recommendation: with Compose Multiplatform 1.9.0, prefer upgrading Material3 Adaptive to org.jetbrains.compose.material3.adaptive:adaptive*:1.2.0 (or at least a 1.2.x compatible version) rather than staying on 1.1.2. [2]
Sources:
[1] MvnRepository – org.jetbrains.compose.material3.adaptive:adaptive:1.1.2 dependencies (shows Jetpack adaptive 1.1.0 + CMP 1.8.2 deps) https://mvnrepository.com/artifact/org.jetbrains.compose.material3.adaptive/adaptive/1.1.2/dependencies
[2] Kotlin docs – Compose Multiplatform 1.9.x deps list (Material3 Adaptive 1.2.0) https://kotlinlang.org/docs/multiplatform/whats-new-compose-190.html
[3] AndroidX release notes – Compose Material3 Adaptive versions (1.1.0, 1.2.0, etc.) https://developer.android.com/jetpack/androidx/releases/compose-material3-adaptive
🌐 Web query:
org.jetbrains.compose.material3.adaptive versions release notes
💡 Result:
Versions published for org.jetbrains.compose.material3.adaptive (Maven Central)
The JetBrains artifacts (for example org.jetbrains.compose.material3.adaptive:adaptive and ...:adaptive-desktop) show these version lines published on Maven Central, up through 1.2.0-alpha03 (dated Jul 4, 2025 on MVNRepository): 1.1.0-alpha01…alpha04, 1.1.0-beta01/beta02, 1.1.0-rc01, 1.1.0, 1.1.1, 1.1.2, then 1.2.0-alpha01…alpha03. [1], [2]
Where to find release notes (what changed in each version)
JetBrains doesn’t appear to publish a separate per-version changelog page specifically for the org.jetbrains.compose.material3.adaptive coordinates; instead:
- Upstream (source of truth for change lists): use the AndroidX Compose Material3 Adaptive release notes (for
androidx.compose.material3.adaptive:adaptive-*), which has detailed per-version “API changes / bug fixes” entries (for example 1.1.0, 1.2.0, 1.3.0-alpha07/alpha08, etc.). [3] - JetBrains mapping notes (what CMP ships): Compose Multiplatform “What’s new” pages note which
org.jetbrains.compose.material3.adaptive:adaptive*version they bundle and what Jetpack version it’s based on (for example, CMP notesadaptive*:1.2.0-alpha03, based on Jetpack Material3 Adaptive 1.1.0). [4]
Sources:
[1] MVNRepository – org.jetbrains.compose.material3.adaptive:adaptive versions list: (mvnrepository.com)
[2] MVNRepository – org.jetbrains.compose.material3.adaptive:adaptive-desktop versions list: (mvnrepository.com)
[3] AndroidX release notes – Compose Material3 Adaptive: (developer.android.com)
[4] JetBrains docs – What’s new in Compose Multiplatform (notes material3.adaptive version/basis): (jetbrains.com)
Citations:
- 1: https://mvnrepository.com/artifact/org.jetbrains.compose.material3.adaptive/adaptive?utm_source=openai
- 2: https://mvnrepository.com/artifact/org.jetbrains.compose.material3.adaptive/adaptive-desktop?utm_source=openai
- 3: https://developer.android.com/jetpack/androidx/releases/compose-material3-adaptive?utm_source=openai
- 4: https://www.jetbrains.com/help/kotlin-multiplatform-dev/whats-new-compose-180.html?utm_source=openai
🏁 Script executed:
# Check the full context of the dependency file
cd cmp-android/dependencies && head -n 410 demoDebugRuntimeClasspath.txt | tail -n 25Repository: openMF/android-client
Length of output: 1483
Upgrade org.jetbrains.compose.material3.adaptive from 1.1.2 to 1.2.0 for alignment with Compose 1.9.0.
The 1.1.2 artifacts are built for Compose ~1.8.2 and are not aligned with the Compose 1.9.0 stack used elsewhere in this classpath. JetBrains recommends org.jetbrains.compose.material3.adaptive:adaptive*:1.2.0 for Compose 1.9.x. Similarly, material3-adaptive-navigation-suite:1.8.2 should be updated to 1.8.2+ that aligns with Compose 1.9.0. Using 1.1.2 with 1.9.0 may work but introduces version mismatch risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmp-android/dependencies/demoDebugRuntimeClasspath.txt` around lines 397 -
400, Bump the three adaptive artifacts
org.jetbrains.compose.material3.adaptive:adaptive-layout,
org.jetbrains.compose.material3.adaptive:adaptive-navigation, and
org.jetbrains.compose.material3.adaptive:adaptive from 1.1.2 to 1.2.0, and
update org.jetbrains.compose.material3:material3-adaptive-navigation-suite
(currently 1.8.2) to a release that aligns with Compose 1.9.0 (use the matching
1.9.x-compatible version), so all compose adaptive artifacts match the Compose
1.9.0 stack and avoid version mismatch.
| androidx.window:window-core-android:1.3.0 | ||
| androidx.window:window-core:1.3.0 | ||
| androidx.window:window:1.3.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file in question to understand the context
head -200 cmp-android/dependencies/demoReleaseRuntimeClasspath.txt | tail -50Repository: openMF/android-client
Length of output: 2095
🏁 Script executed:
# Also check around line 386 to see the org.jetbrains.androidx.window dependency
sed -n '380,395p' cmp-android/dependencies/demoReleaseRuntimeClasspath.txtRepository: openMF/android-client
Length of output: 1028
🏁 Script executed:
# Find build files that declare these dependencies
find . -name "build.gradle*" -o -name "*.gradle.kts" | head -20Repository: openMF/android-client
Length of output: 738
🏁 Script executed:
# Search for declarations of these specific window dependencies
rg -n 'org\.jetbrains\.androidx\.window|androidx\.window:window-core' --type=gradle --type=kotlinRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check for any documentation or comments about dependency scoping
rg -n 'org\.jetbrains\.androidx\.window' -C 5Repository: openMF/android-client
Length of output: 5281
🏁 Script executed:
# Check the cmp-android build.gradle.kts to see explicit dependencies
cat -n cmp-android/build.gradle.kts | head -150Repository: openMF/android-client
Length of output: 5880
🏁 Script executed:
# Look for multiplatform dependencies or version catalogs that might declare these
rg -n 'window|Window' cmp-android/build.gradle.kts -C 2Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check if there's a version catalog or dependencies file
find . -name "*.toml" -o -name "libs.versions.toml" | xargs ls -la 2>/dev/nullRepository: openMF/android-client
Length of output: 142
🏁 Script executed:
# Check the root build.gradle.kts for shared dependency declarations
cat -n build.gradle.kts | head -100Repository: openMF/android-client
Length of output: 2405
🏁 Script executed:
# Look at cmp-shared build config as it might be the source of dependencies
cat -n cmp-shared/build.gradle.kts | head -150Repository: openMF/android-client
Length of output: 2175
🏁 Script executed:
# Check the version catalog for window and compose dependencies
rg -n 'window|compose' gradle/libs.versions.toml -i -C 2 | head -100Repository: openMF/android-client
Length of output: 5306
🏁 Script executed:
# Look for the convention plugins that might pull in these dependencies
find . -path "*/build-logic/*" -name "*.kt" -o -name "*.gradle.kts" | xargs rg -l "jetbrains|androidx.window" 2>/dev/null | head -5Repository: openMF/android-client
Length of output: 213
🏁 Script executed:
# Check the Android convention plugin
cat -n build-logic/convention/build.gradle.kts | head -50Repository: openMF/android-client
Length of output: 1876
🏁 Script executed:
# Look for AndroidCompose or Jetbrains Compose convention plugin implementations
find build-logic -name "*.kt" | xargs rg -l "jetbrainsCompose|JetbrainsCompose" 2>/dev/nullRepository: openMF/android-client
Length of output: 49
🏁 Script executed:
# List the convention plugins in build-logic
find build-logic -name "*.kt" -type f | head -20Repository: openMF/android-client
Length of output: 1492
🏁 Script executed:
# Check the Android Compose convention plugin
cat build-logic/convention/src/main/kotlin/AndroidApplicationComposeConventionPlugin.kt 2>/dev/null | head -100Repository: openMF/android-client
Length of output: 685
🏁 Script executed:
# Look for dependencies on org.jetbrains.androidx in the cmp modules
rg -n 'org\.jetbrains' gradle/libs.versions.toml -C 2Repository: openMF/android-client
Length of output: 6250
🏁 Script executed:
# Check if cmp-navigation or cmp-shared declares org.jetbrains.androidx dependencies
rg -n 'jetbrains' cmp-navigation/build.gradle.kts cmp-shared/build.gradle.kts -C 2Repository: openMF/android-client
Length of output: 384
🏁 Script executed:
# Look at the full libs.versions.toml to understand the Jetbrains artifacts section
sed -n '145,180p' gradle/libs.versions.tomlRepository: openMF/android-client
Length of output: 1027
🏁 Script executed:
# Check if there are any exclusions or dependency configurations for window artifacts
rg -n 'window|Window' cmp-shared/build.gradle.kts cmp-navigation/build.gradle.kts -C 3Repository: openMF/android-client
Length of output: 626
🏁 Script executed:
# Look for any explicit dependencies on org.jetbrains.androidx.window in the codebase
rg -n 'window' gradle/libs.versions.toml | grep -i jetbrainsRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check what declares the JetBrains window artifact by looking at Jetbrains Compose transitive deps
# Look at cmp-navigation to see what it depends on
cat -n cmp-navigation/build.gradle.kts | head -100Repository: openMF/android-client
Length of output: 3483
🏁 Script executed:
# Check if there's any documentation or comments about this in the README or docs
find . -name "README*" -o -name "*.md" | xargs rg -l "multiplatform\|Jetbrains\|window" -i 2>/dev/null | head -5Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Let's examine what cmp-shared explicitly imports from jetbrains libraries
rg -n 'org\.jetbrains' gradle/libs.versions.toml | grep -v '^#'Repository: openMF/android-client
Length of output: 4588
🏁 Script executed:
# Find the definition of libs.window.size
rg -n 'window\.size' gradle/libs.versions.tomlRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Search for all window-related library definitions
rg -n 'window' gradle/libs.versions.toml | grep -v '^#'Repository: openMF/android-client
Length of output: 515
🏁 Script executed:
# Check if org.jetbrains.androidx.window.window-core is explicitly declared anywhere
rg -n 'org\.jetbrains\.androidx\.window' gradle/libs.versions.tomlRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Let's also check the Jetbrains Compose metadata to understand transitive deps
# Look at what's in the Android lib folder if deps are cached
find . -path "*/androidx/window*" 2>/dev/null | head -10Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Look for dependency constraint or exclusion rules
rg -n 'exclude|constraint|transitive' cmp-android/build.gradle.kts cmp-shared/build.gradle.kts cmp-navigation/build.gradle.ktsRepository: openMF/android-client
Length of output: 150
🏁 Script executed:
# Check if there are any dependency management or BOM declarations
rg -n 'dependencyManagement|constraints|platform' cmp-android/build.gradle.kts cmp-shared/build.gradle.ktsRepository: openMF/android-client
Length of output: 150
🏁 Script executed:
# Check the Jetbrains Compose plugin version and see if there's documentation about it
rg -n 'compose-plugin' gradle/libs.versions.toml -C 2Repository: openMF/android-client
Length of output: 1155
🏁 Script executed:
# Look at what the Jetbrains Compose plugin pulls in for Android by checking configuration details
cat cmp-shared/build.gradle.kts | grep -A 20 "kotlin {"Repository: openMF/android-client
Length of output: 652
🏁 Script executed:
# Search for any known Gradle configuration that separates Android from other targets
rg -n 'androidMain|jvmMain|commonMain' cmp-shared/build.gradle.kts cmp-navigation/build.gradle.ktsRepository: openMF/android-client
Length of output: 267
🏁 Script executed:
# Check if there's any documentation or comments about Jetbrains Compose multiplatform setup
rg -n 'jetbrainsCompose|Jetbrains.*Compose' build-logic -C 3 | head -50Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Look at the KMP library convention plugin to see if it has special handling
cat -n build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.ktRepository: openMF/android-client
Length of output: 2144
🏁 Script executed:
# Check the CMP feature convention plugin
cat -n build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt 2>/dev/null | head -80Repository: openMF/android-client
Length of output: 3617
🏁 Script executed:
# Search for any known issues or TODOs related to window dependencies
rg -n 'TODO|FIXME|BUG|window.*conflict|window.*duplicate' . --type=kotlin --type=gradle -i 2>/dev/null | head -20Repository: openMF/android-client
Length of output: 2721
🏁 Script executed:
# Let's verify if the org.jetbrains.androidx.window actually contains androidx.window classes
# by checking Maven Central documentation or searching for package info
web_search "org.jetbrains.androidx.window:window-core pom dependencies"Repository: openMF/android-client
Length of output: 115
🌐 Web query:
org.jetbrains.androidx.window vs androidx.window kotlin multiplatform compose duplicate classes
💡 Result:
org.jetbrains.androidx.window:* and androidx.window:* contain (largely) the same androidx.window… packages. In Compose/Kotlin Multiplatform, JetBrains publishes “multiplatform-packaged” Jetpack libraries under org.jetbrains.androidx.* so you can use them from commonMain (non‑Android targets get JetBrains-built artifacts; Android gets the Android artifact). If you also add the original androidx.window:* artifacts, Gradle ends up with two jars containing the same classes, producing Duplicate class androidx.window… errors. [1]
What to do
Pick one coordinate set and use it consistently:
Option A (typical for Compose Multiplatform): use JetBrains-packaged artifacts
commonMain.dependencies {
implementation("org.jetbrains.androidx.window:window-core:<version>")
// (and/or other org.jetbrains.androidx.window modules)
}And remove any implementation("androidx.window:window-…") you added. [1][2]
If androidx.window:* is coming transitively from another dependency, exclude it:
implementation("some:dep:1.0") {
exclude(group = "androidx.window")
}Option B: use AndroidX WindowManager directly (Android-only / limited KMP surface)
AndroidX WindowManager added KMP support for some APIs (for example Window Size Classes in 1.3.0). If you go this route, depend on androidx.window:* and do not bring in org.jetbrains.androidx.window:* in the same compilation. [3]
Rule of thumb:
commonMainin Compose Multiplatform → preferorg.jetbrains.androidx.*and don’t also add the matchingandroidx.*. [1]- If you explicitly add both, you’ll hit duplicate classes.
Sources: [1] Kotlin MPP docs on how Jetpack libraries are packaged; [2] JetBrains “What’s new” noting org.jetbrains.androidx.window:window-core; [3] AndroidX WindowManager release notes mentioning KMP support.
[1] (kotlinlang.org)
[2] (jetbrains.com)
[3] (developer.android.com)
Citations:
- 1: https://kotlinlang.org/docs/multiplatform/compose-multiplatform-jetpack-libraries.html?utm_source=openai
- 2: https://www.jetbrains.com/help/kotlin-multiplatform-dev/whats-new-compose-180.html?utm_source=openai
- 3: https://developer.android.com/jetpack/androidx/releases/window?utm_source=openai
🏁 Script executed:
# Search for explicit androidx.window declarations in the codebase
rg -n 'androidx\.window' gradle/libs.versions.toml cmp-android/build.gradle.kts cmp-shared/build.gradle.kts cmp-navigation/build.gradle.ktsRepository: openMF/android-client
Length of output: 197
🏁 Script executed:
# Check if libs.window.size from material3-window-size-class pulls androidx.window
# First, see what that library depends on
cat -n gradle/libs.versions.toml | grep -A 2 -B 2 "window-size"Repository: openMF/android-client
Length of output: 903
🏁 Script executed:
# Look for any dependency on androidx.window:window (not just window-core)
rg -n 'window:window' gradle/libs.versions.tomlRepository: openMF/android-client
Length of output: 171
🏁 Script executed:
# Check the explicit dependencies in cmp-shared and cmp-navigation that might pull window
cat -n cmp-shared/build.gradle.kts | sed -n '38,50p'Repository: openMF/android-client
Length of output: 696
🏁 Script executed:
# Check if androidx-window-manager is actually used in any build file
rg -n 'androidx-window-manager|window.manager' cmp-android/build.gradle.kts cmp-shared/build.gradle.kts cmp-navigation/build.gradle.kts core-base/*/build.gradle.kts core/*/build.gradle.kts feature/*/build.gradle.kts -iRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Let's check what compose.material3 (from Jetbrains) depends on by looking at Gradle metadata
# First, see if there's any gradle metadata or pom files cached
find . -path "*compose*material3*" -name "*.pom" 2>/dev/null | head -3Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check all explicit dependencies for window in all build files
find . -name "build.gradle.kts" -exec rg -l 'window' {} \; 2>/dev/nullRepository: openMF/android-client
Length of output: 208
🏁 Script executed:
# Now let's find where androidx.window:window-core or similar is declared
for file in $(find . -name "build.gradle.kts" | head -30); do
if rg -q 'window' "$file" 2>/dev/null; then
echo "=== $file ==="
rg -n 'window' "$file" -i
fi
doneRepository: openMF/android-client
Length of output: 398
🏁 Script executed:
# Let's also check if the issue is that compose.material3 (from Compose BOM) depends on androidx.window
# Check what the demoReleaseRuntimeClasspath reveals about which artifact depends on window-core
rg -n 'androidx.window|org.jetbrains.androidx.window' cmp-android/dependencies/demoReleaseRuntimeClasspath.txtRepository: openMF/android-client
Length of output: 280
🏁 Script executed:
# Let's check what dev.chrisbanes.material3:material3-window-size-class-multiplatform depends on
# by looking at its transitive dependencies in the classpath
# First verify it's in the classpath
rg -n 'dev.chrisbanes' cmp-android/dependencies/demoReleaseRuntimeClasspath.txtRepository: openMF/android-client
Length of output: 226
🏁 Script executed:
# Look at the lines around the window artifacts to understand the dependency order
sed -n '168,180p' cmp-android/dependencies/demoReleaseRuntimeClasspath.txtRepository: openMF/android-client
Length of output: 553
🏁 Script executed:
# Check if there's an explicit dependency on androidx.window anywhere that should be excluded
rg -n 'androidx-window-manager' gradle/libs.versions.toml cmp-android/build.gradle.ktsRepository: openMF/android-client
Length of output: 197
🏁 Script executed:
# Let's trace the actual dependency declarations to understand the flow
# Check if core/designsystem or core-base/designsystem explicitly declare the androidx window artifacts
cat -n core/designsystem/build.gradle.kts | head -80Repository: openMF/android-client
Length of output: 2802
🏁 Script executed:
# Check the dependency in cmp-navigation more carefully
cat -n cmp-navigation/build.gradle.kts | sed -n '50,70p'Repository: openMF/android-client
Length of output: 1048
🏁 Script executed:
# Check if there are any existing exclude rules for window artifacts
rg -n 'exclude.*window|window.*exclude' cmp-android/build.gradle.kts cmp-shared/build.gradle.kts cmp-navigation/build.gradle.kts core/designsystem/build.gradle.kts core-base/designsystem/build.gradle.kts -iRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check if the dev.chrisbanes library has any explicit configuration or exclusion
rg -n 'dev.chrisbanes|window.size' cmp-android/build.gradle.kts cmp-shared/build.gradle.kts cmp-navigation/build.gradle.kts core/designsystem/build.gradle.kts core-base/designsystem/build.gradle.kts -B 2 -A 2Repository: openMF/android-client
Length of output: 1313
🏁 Script executed:
# Verify which artifacts are coming from dev.chrisbanes vs Jetbrains Compose
# Look at the full context around window artifacts in the classpath
sed -n '165,200p' cmp-android/dependencies/demoReleaseRuntimeClasspath.txt | grep -E 'androidx|jetbrains'Repository: openMF/android-client
Length of output: 469
🏁 Script executed:
# Check if androidx-window-manager library alias is actually used anywhere
rg -n 'androidx-window-manager' . --type=gradle --type=kotlinRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Let's check the dev.chrisbanes library to understand its dependencies better
# Look at what versions are available and if there are any alternatives
rg -n 'material3-window-size-class' gradle/libs.versions.tomlRepository: openMF/android-client
Length of output: 377
🏁 Script executed:
# Check if there's a way to exclude androidx.window from dev.chrisbanes library
# Let's see the exact implementation in cmp-navigation and designsystem files
grep -A 10 'implementation(libs.window.size)' cmp-navigation/build.gradle.ktsRepository: openMF/android-client
Length of output: 425
🏁 Script executed:
# Check if there's any known configuration to handle this
# Look at the CMPFeatureConventionPlugin to see if it has any window handling
cat -n build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt | tail -20Repository: openMF/android-client
Length of output: 1447
Resolve the androidx.window duplicate class conflict in Compose Multiplatform setup.
Both androidx.window:window-core* (lines 172-173) and org.jetbrains.androidx.window:window-core (line 386) package identical androidx.window classes, causing duplicate class errors. The root cause is dev.chrisbanes.material3:material3-window-size-class-multiplatform (used in cmp-navigation, core/designsystem, core-base/designsystem) pulling AndroidX window artifacts while your Compose Multiplatform setup expects JetBrains-packaged artifacts.
Fix options:
- Exclude
androidx.windowfromlibs.window.sizewhere declared:implementation(libs.window.size) { exclude(group = "androidx.window") } - Or upgrade the dev.chrisbanes library to a version that depends on
org.jetbrains.androidx.windowinstead, or switch to a Compose Multiplatform–compatible alternative.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmp-android/dependencies/demoReleaseRuntimeClasspath.txt` around lines 172 -
174, The duplicate androidx.window classes are introduced by
dev.chrisbanes.material3:material3-window-size-class-multiplatform pulled via
libs.window.size into modules (cmp-navigation, core/designsystem,
core-base/designsystem); fix by excluding the androidx.window group from the
dependency declaration for libs.window.size in those modules (i.e., add an
exclusion for group "androidx.window" on the implementation of libs.window.size)
or alternatively update/replace
dev.chrisbanes.material3:material3-window-size-class-multiplatform with a
version or alternative that depends on org.jetbrains.androidx.window
(JetBrains-packaged) to avoid the conflicting androidx.window artifacts.
| androidx.compose.material3.adaptive:adaptive-android:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-layout-android:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-layout:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-navigation-android:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive-navigation:1.1.0 | ||
| androidx.compose.material3.adaptive:adaptive:1.1.0 | ||
| androidx.compose.material3:material3-adaptive-navigation-suite-android:1.3.2 | ||
| androidx.compose.material3:material3-adaptive-navigation-suite:1.3.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
androidx.compose.material3.adaptive 1.1.0 compose-bom 2025.01.00 compatibility
💡 Result:
androidx.compose.material3.adaptive:adaptive-*:1.1.0 (released March 12, 2025) is not what you get with androidx.compose:compose-bom:2025.01.00. The Compose BOM 2025.01.00 predates that release and maps the Material3 Adaptive artifacts to the 1.0.x line (shown as 1.0.0 in the BOM mapping table), while 1.1.0 only appears in later BOMs (and newer BOMs map to 1.2.0, etc.). [1][2]
What to do
- If you want Material3 Adaptive 1.1.0, use a BOM version that maps to it (per the BOM mapping page, that’s a later 2025 BOM than 2025.01.00). [1]
- If you keep BOM 2025.01.00 and force
adaptive-*:1.1.0manually, you’re overriding the BOM, which can work but may introduce version skew across Compose libraries. [3]
Sources
- Android Developers — BOM to library version mapping (shows which library versions each Compose BOM maps to, including
androidx.compose.material3.adaptive). (developer.android.com) - Android Developers — Compose Material3 Adaptive release notes (shows
1.1.0release date). (developer.android.com) - Android Developers — Use a Bill of Materials (explains BOM usage and overriding versions). (developer.android.com)
Citations:
- 1: https://developer.android.com/develop/ui/compose/bom/bom-mapping?utm_source=openai
- 2: https://developer.android.com/jetpack/androidx/releases/compose-material3-adaptive?utm_source=openai
- 3: https://developer.android.com/develop/ui/compose/bom?utm_source=openai
🌐 Web query:
material3-adaptive-navigation-suite 1.3.2 androidx.compose.material3 1.3.2 compatibility
💡 Result:
Yes—androidx.compose.material3:material3-adaptive-navigation-suite:1.3.2 is intended to be used with the rest of the Compose Material3 1.3.2 artifacts (including androidx.compose.material3:material3:1.3.2). Google’s release notes treat material3-* as a single versioned set, and 1.3.2 is one such coordinated release. [1]
Recommended: keep them on the same version (or use the Compose BOM so they’re aligned automatically). The Compose BOM maps the Material3 group to version 1.3.2 for BOMs that include that set. [2]
Example (no BOM):
implementation("androidx.compose.material3:material3:1.3.2")
implementation("androidx.compose.material3:material3-adaptive-navigation-suite:1.3.2")Sources: [1] [2]
Material3 Adaptive 1.1.0 overrides compose-bom 2025.01.00, creating version skew. The Compose BOM 2025.01.00 maps androidx.compose.material3.adaptive:* to 1.0.0, not 1.1.0 (released March 2025). Using 1.1.0 manually overrides the BOM, which can introduce binary incompatibilities across Compose libraries. Either use a later Compose BOM version that maps to 1.1.0 or revert these artifacts to 1.0.0. The material3-adaptive-navigation-suite:1.3.2 is correctly aligned with material3:1.3.2.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmp-android/dependencies/prodDebugRuntimeClasspath.txt` around lines 23 - 30,
The Compose Adaptive artifacts listed
(androidx.compose.material3.adaptive:adaptive-*, adaptive,
adaptive-layout(-android), adaptive-navigation(-android) at 1.1.0) are
overriding the Compose BOM (2025.01.00) which pins them to 1.0.0; either update
the BOM to a version that maps these adaptive artifacts to 1.1.0 or revert these
artifact coordinates back to 1.0.0 to eliminate version skew. Locate the
dependency declarations that add androidx.compose.material3.adaptive:adaptive,
adaptive-layout, adaptive-layout-android, adaptive-navigation,
adaptive-navigation-android, adaptive-android and change their versions to
1.0.0, or alternatively bump the compose-bom version in your dependency
management to a release that officially maps these adaptive artifacts to 1.1.0
so all Compose modules remain aligned. Ensure
material3-adaptive-navigation-suite:1.3.2 remains unchanged if already aligned
with material3:1.3.2.
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.unit.dp | ||
| import com.mifos.core.designsystem.theme.DesignToken |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual content of PreviewPage.kt at the relevant lines
fd "PreviewPage.kt" --type f | head -5Repository: openMF/android-client
Length of output: 598
🏁 Script executed:
# Get the file and check lines around 42 and 62
find . -name "PreviewPage.kt" -path "*createShareAccount*" -type f | head -1Repository: openMF/android-client
Length of output: 168
🏁 Script executed:
# Once we have the file path, examine it
cat -n "$(find . -name 'PreviewPage.kt' -path '*createShareAccount*' -type f | head -1)" | sed -n '30,75p'Repository: openMF/android-client
Length of output: 2368
🏁 Script executed:
# Search for largeIncreased in the codebase
rg "largeIncreased" --type=kt -B2 -A2Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Check KptTheme spacing definitions
rg "spacing\s*=" --type=kt -g "*KptTheme*" -B2 -A2Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Check if DesignToken.spacing.largeIncreased is still used anywhere
rg "DesignToken\.spacing\.largeIncreased" --type=ktRepository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Search for largeIncreased definition
rg "largeIncreased" -B2 -A2Repository: openMF/android-client
Length of output: 42072
🏁 Script executed:
# Find KptTheme file to see spacing tokens
fd "KptTheme" --type f -e ".kt"Repository: openMF/android-client
Length of output: 252
🏁 Script executed:
# Look at KptTheme spacing definitions
find . -name "KptTheme*" -type f | head -5Repository: openMF/android-client
Length of output: 256
🏁 Script executed:
# Search for spacing object definitions in KptTheme or similar
rg "spacing\s*=" -B1 -A5 | head -100Repository: openMF/android-client
Length of output: 5471
🏁 Script executed:
# Check DesignToken.spacing definitions
rg "object.*spacing" -B2 -A10Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check KptTheme file to see spacing options
cat -n ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptTheme.kt | head -150Repository: openMF/android-client
Length of output: 2981
🏁 Script executed:
# Check for spacing interface/class definitions
rg "spacing\." --type=kt -g "*KptTheme*" | head -40Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
# Look for KptSpacing definition
fd "KptSpacing" --type f -e ".kt"Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check the KptThemeExtensions file
cat -n ./core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/KptThemeExtensions.kt | head -200Repository: openMF/android-client
Length of output: 8993
🏁 Script executed:
# Find KptSpacing interface
fd "KptSpacing" --type f | xargs cat | head -200Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Search for spacing token definitions in the interface
rg "interface KptSpacing|val.*Dp" -A 50 -B 2 | head -150Repository: openMF/android-client
Length of output: 19134
🏁 Script executed:
# Look for KptSpacingImpl
find . -name "*KptSpacing*" -type fRepository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Check if other PreviewPage files in the PR have the same pattern
rg "DesignToken.spacing.largeIncreased" --type=kotlin | grep -i previewRepository: openMF/android-client
Length of output: 830
🏁 Script executed:
# Verify the exact KptTheme spacing options available
rg "override val.*Dp.*=" core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/theme/KptColorSchemeImpl.kt | grep -A6 "KptSpacingImpl"Repository: openMF/android-client
Length of output: 47
Incomplete migration: DesignToken.spacing.largeIncreased remains in place with no KptTheme equivalent.
Line 62 still references DesignToken.spacing.largeIncreased (20.dp), while the rest of the file uses KptTheme. KptTheme spacing tokens available are: xs (4dp), sm (8dp), md (16dp), lg (24dp), xl (32dp), xxl (64dp)—there is no exact 20.dp equivalent. This requires either migrating to the closest KptTheme token (lg at 24.dp or md at 16.dp, both of which change the spacing by 4dp) or confirming the 20.dp value should be preserved. Similar PreviewPage files have the same pattern, suggesting either a deliberate decision to keep this value or an incomplete migration across the codebase.
There was a problem hiding this comment.
This is intentional
There was a problem hiding this comment.
@kartikey004, thank you for confirming! I've noted that retaining DesignToken.spacing.largeIncreased for the vertical arrangement spacing at line 62 is an intentional design decision in this migration.
✏️ Learnings added
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt:42-42
Timestamp: 2026-02-17T14:39:37.695Z
Learning: In PR `#2610` (KptTheme migration), the PreviewPage.kt file in createShareAccount intentionally retains `DesignToken.spacing.largeIncreased` (20.dp) for the verticalArrangement spacing at line 62 within the scrollable Column, even though other spacing tokens in the file were migrated to KptTheme. This is a deliberate choice to preserve the specific 20.dp spacing between content sections, as KptTheme does not offer an exact 20.dp equivalent.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt:56-61
Timestamp: 2026-02-16T09:59:35.731Z
Learning: In PR `#2610` (KptTheme migration), the PreviewPage.kt outer Column bottom padding was intentionally reduced from DesignToken.padding.large to KptTheme.spacing.sm (Line 57) as a deliberate UI improvement to tighten the layout, not as a direct token-for-token replacement. This spacing reduction improves the overall visual design of the share account preview screen.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt:98-109
Timestamp: 2026-02-16T08:25:10.251Z
Learning: In the DesignToken.kt file for the android-client project, the team prefers to use raw dp-based naming (dp1, dp2, dp30, dp40, dp44, etc.) for spacing tokens rather than semantic names. This naming convention should be accepted in design token definitions.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt:286-289
Timestamp: 2026-02-16T09:01:46.256Z
Learning: In the android-client project PR `#2610` (KptTheme migration), the typography hierarchy from MifosTypography has been simplified when migrating to KptTheme. The `titleMediumEmphasized` style (FontWeight.SemiBold/600) maps to `KptTheme.typography.titleMedium` (FontWeight.Medium/500), representing an intentional design decision to reduce visual emphasis across dialog headers and similar UI elements.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt:328-337
Timestamp: 2026-02-16T08:26:24.063Z
Learning: In the DesignToken.kt file for the android-client project, the team prefers to use raw dp-based naming (dp5, dp18, dp20, dp42, dp48, dp72, dp100, dp120, dp128, etc.) for size tokens in the AppSizes data class rather than only semantic names. This naming convention should be accepted in design token definitions.
Learnt from: amanna13
Repo: openMF/android-client PR: 2569
File: core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/layout/KptResponsiveLayout.kt:140-153
Timestamp: 2025-12-31T08:19:28.678Z
Learning: In the android-client project, when syncing template code from kmp-project-template, functional or behavioral fixes in the synced template code are intentionally deferred to follow-up issues to keep the sync PR scoped to template syncing only.
| ) { | ||
| Text(text = label, style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.surfaceVariant) | ||
| Text(text = value, style = MaterialTheme.typography.bodyMedium, fontWeight = FontWeight.SemiBold) | ||
| Text(text = label, style = KptTheme.typography.bodyMedium, color = KptTheme.colorScheme.surfaceVariant) |
There was a problem hiding this comment.
Bug: surfaceVariant used as text color instead of onSurfaceVariant.
KptTheme.colorScheme.surfaceVariant is a surface/background color, not a content color. Using it for text will likely produce very low or no contrast against the sheet background. The correct token is onSurfaceVariant, which you already use correctly on Line 458 (FarApartTextItem) and Line 609 (SuccessBottomSheet).
🐛 Fix: use `onSurfaceVariant` for label text color
- Text(text = label, style = KptTheme.typography.bodyMedium, color = KptTheme.colorScheme.surfaceVariant)
+ Text(text = label, style = KptTheme.typography.bodyMedium, color = KptTheme.colorScheme.onSurfaceVariant)📝 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.
| Text(text = label, style = KptTheme.typography.bodyMedium, color = KptTheme.colorScheme.surfaceVariant) | |
| Text(text = label, style = KptTheme.typography.bodyMedium, color = KptTheme.colorScheme.onSurfaceVariant) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`
at line 568, The label Text in LoanRepaymentScreen uses
KptTheme.colorScheme.surfaceVariant (a background color) which causes poor
contrast; update the Text call that uses label with style
KptTheme.typography.bodyMedium to use KptTheme.colorScheme.onSurfaceVariant
instead of surfaceVariant so the text uses the correct content color (match
other usages like FarApartTextItem and SuccessBottomSheet).
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(4.dp), | ||
| .padding(KptTheme.spacing.xs), | ||
| ) { | ||
| Text( | ||
| modifier = Modifier.weight(2.5f), | ||
| text = transaction.principalPortion.toString(), | ||
| style = MaterialTheme.typography.bodyLarge, | ||
| color = MaterialTheme.colorScheme.onBackground, | ||
| style = KptTheme.typography.bodyLarge, | ||
| color = KptTheme.colorScheme.onSurfaceVariant, | ||
| textAlign = TextAlign.Start, | ||
| ) | ||
|
|
||
| Text( | ||
| modifier = Modifier | ||
| .weight(2.5f) | ||
| .padding(horizontal = 4.dp), | ||
| .padding(horizontal = KptTheme.spacing.xs), | ||
| text = transaction.interestPortion.toString(), | ||
| style = MaterialTheme.typography.bodyLarge, | ||
| color = MaterialTheme.colorScheme.onBackground, | ||
| style = KptTheme.typography.bodyLarge, | ||
| color = KptTheme.colorScheme.onSurfaceVariant, | ||
| textAlign = TextAlign.Center, | ||
| ) | ||
|
|
||
| Text( | ||
| modifier = Modifier.weight(2.5f), | ||
| text = transaction.feeChargesPortion.toString(), | ||
| style = MaterialTheme.typography.bodyLarge, | ||
| color = MaterialTheme.colorScheme.onBackground, | ||
| style = KptTheme.typography.bodyLarge, | ||
| color = KptTheme.colorScheme.onBackground, | ||
| textAlign = TextAlign.Center, | ||
| ) | ||
|
|
||
| Text( | ||
| modifier = Modifier | ||
| .weight(2.5f) | ||
| .padding(start = 4.dp), | ||
| .padding(start = KptTheme.spacing.xs), | ||
| text = transaction.penaltyChargesPortion.toString(), | ||
| style = MaterialTheme.typography.bodyLarge, | ||
| color = MaterialTheme.colorScheme.onBackground, | ||
| style = KptTheme.typography.bodyLarge, | ||
| color = KptTheme.colorScheme.onBackground, | ||
| textAlign = TextAlign.End, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Data-row text color inconsistency: onSurfaceVariant vs onBackground on the same surfaceVariant card.
principalPortion and interestPortion correctly use onSurfaceVariant (Lines 365, 375), but feeChargesPortion and penaltyChargesPortion switch to onBackground (Lines 383, 393). All four sit in the same Row on a surfaceVariant container, so they should use the same semantic color. With a custom theme where onBackground ≠ onSurfaceVariant, this will produce visibly mismatched text in the same row.
Proposed fix — unify to `onSurfaceVariant`
Text(
modifier = Modifier.weight(2.5f),
text = transaction.feeChargesPortion.toString(),
style = KptTheme.typography.bodyLarge,
- color = KptTheme.colorScheme.onBackground,
+ color = KptTheme.colorScheme.onSurfaceVariant,
textAlign = TextAlign.Center,
)
Text(
modifier = Modifier
.weight(2.5f)
.padding(start = KptTheme.spacing.xs),
text = transaction.penaltyChargesPortion.toString(),
style = KptTheme.typography.bodyLarge,
- color = KptTheme.colorScheme.onBackground,
+ color = KptTheme.colorScheme.onSurfaceVariant,
textAlign = TextAlign.End,
)📝 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.
| Row( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .padding(4.dp), | |
| .padding(KptTheme.spacing.xs), | |
| ) { | |
| Text( | |
| modifier = Modifier.weight(2.5f), | |
| text = transaction.principalPortion.toString(), | |
| style = MaterialTheme.typography.bodyLarge, | |
| color = MaterialTheme.colorScheme.onBackground, | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onSurfaceVariant, | |
| textAlign = TextAlign.Start, | |
| ) | |
| Text( | |
| modifier = Modifier | |
| .weight(2.5f) | |
| .padding(horizontal = 4.dp), | |
| .padding(horizontal = KptTheme.spacing.xs), | |
| text = transaction.interestPortion.toString(), | |
| style = MaterialTheme.typography.bodyLarge, | |
| color = MaterialTheme.colorScheme.onBackground, | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onSurfaceVariant, | |
| textAlign = TextAlign.Center, | |
| ) | |
| Text( | |
| modifier = Modifier.weight(2.5f), | |
| text = transaction.feeChargesPortion.toString(), | |
| style = MaterialTheme.typography.bodyLarge, | |
| color = MaterialTheme.colorScheme.onBackground, | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onBackground, | |
| textAlign = TextAlign.Center, | |
| ) | |
| Text( | |
| modifier = Modifier | |
| .weight(2.5f) | |
| .padding(start = 4.dp), | |
| .padding(start = KptTheme.spacing.xs), | |
| text = transaction.penaltyChargesPortion.toString(), | |
| style = MaterialTheme.typography.bodyLarge, | |
| color = MaterialTheme.colorScheme.onBackground, | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onBackground, | |
| textAlign = TextAlign.End, | |
| ) | |
| } | |
| Row( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .padding(KptTheme.spacing.xs), | |
| ) { | |
| Text( | |
| modifier = Modifier.weight(2.5f), | |
| text = transaction.principalPortion.toString(), | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onSurfaceVariant, | |
| textAlign = TextAlign.Start, | |
| ) | |
| Text( | |
| modifier = Modifier | |
| .weight(2.5f) | |
| .padding(horizontal = KptTheme.spacing.xs), | |
| text = transaction.interestPortion.toString(), | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onSurfaceVariant, | |
| textAlign = TextAlign.Center, | |
| ) | |
| Text( | |
| modifier = Modifier.weight(2.5f), | |
| text = transaction.feeChargesPortion.toString(), | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onSurfaceVariant, | |
| textAlign = TextAlign.Center, | |
| ) | |
| Text( | |
| modifier = Modifier | |
| .weight(2.5f) | |
| .padding(start = KptTheme.spacing.xs), | |
| text = transaction.penaltyChargesPortion.toString(), | |
| style = KptTheme.typography.bodyLarge, | |
| color = KptTheme.colorScheme.onSurfaceVariant, | |
| textAlign = TextAlign.End, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`
around lines 356 - 396, The two Text composables showing
transaction.feeChargesPortion and transaction.penaltyChargesPortion use
KptTheme.colorScheme.onBackground while the other columns use onSurfaceVariant,
causing a mismatched row; update the color prop on the Texts that render
feeChargesPortion and penaltyChargesPortion to
KptTheme.colorScheme.onSurfaceVariant so all four columns in the Row use the
same semantic color (look for the Text instances referencing
transaction.feeChargesPortion and transaction.penaltyChargesPortion and change
their color).
| ) | ||
| } | ||
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the file structure and find ExperimentalMaterial3Api usage
echo "=== Checking ExperimentalMaterial3Api usage ==="
rg -n "ExperimentalMaterial3Api" --type=kt feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt
echo -e "\n=== Checking OutlinedCard usage ==="
rg -n "OutlinedCard" --type=kt feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt -B2 -A2
echo -e "\n=== Finding build.gradle files for Material3 version ==="
fd "build.gradle" -o "build.gradle.kts" | head -10 | xargs grep -i "material3" 2>/dev/nullRepository: openMF/android-client
Length of output: 432
🏁 Script executed:
#!/bin/bash
# Read the specific file to see the function structure and `@OptIn` placement
echo "=== Reading OfflineDashboardScreen.kt lines 75-200 to see function structure ==="
wc -l feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.ktRepository: openMF/android-client
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Read the file to see structure and `@OptIn` placement
echo "=== Reading OfflineDashboardScreen.kt ==="
cat -n feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt | head -250Repository: openMF/android-client
Length of output: 11047
🏁 Script executed:
#!/bin/bash
# Search for ExperimentalMaterial3Api and OutlinedCard in the file
echo "=== Searching for ExperimentalMaterial3Api ==="
grep -n "ExperimentalMaterial3Api" feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt
echo -e "\n=== Searching for OutlinedCard ==="
grep -n "OutlinedCard" feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt
echo -e "\n=== Searching for OfflineDashboardItemCard function ==="
grep -n "OfflineDashboardItemCard" feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.ktRepository: openMF/android-client
Length of output: 489
🏁 Script executed:
#!/bin/bash
# Find gradle files and check Material3 version
echo "=== Finding build.gradle files ==="
find . -name "build.gradle*" -type f | head -10 | xargs grep -h "material3" 2>/dev/null | head -20Repository: openMF/android-client
Length of output: 241
Move @OptIn(ExperimentalMaterial3Api::class) to OfflineDashboardItemCard or remove if no longer needed.
The @OptIn annotation is placed on OfflineDashboardScreen (line 85), but OutlinedCard with onClick — the experimental API — is used in OfflineDashboardItemCard (line 178), a separate private function. In Kotlin, @OptIn only suppresses warnings within the annotated function's scope and does not propagate to other top-level functions. The annotation should be placed on the function that directly uses the experimental API. If the code compiles, this may indicate that OutlinedCard(onClick) has graduated from experimental status in the current Material3 version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt`
at line 85, The `@OptIn`(ExperimentalMaterial3Api::class) annotation is applied to
OfflineDashboardScreen but the experimental API usage (OutlinedCard(onClick)) is
inside the private function OfflineDashboardItemCard; move the `@OptIn` annotation
from OfflineDashboardScreen to the OfflineDashboardItemCard function (or remove
the annotation entirely if OutlinedCard(onClick) is no longer experimental in
your Material3 version) so the opt-in scope correctly covers the usage site.
| @@ -43,30 +43,31 @@ import com.mifos.core.common.utils.CurrencyFormatter | |||
| import com.mifos.core.common.utils.DateHelper | |||
| import com.mifos.core.designsystem.component.MifosBottomSheet | |||
| import com.mifos.core.designsystem.theme.DesignToken | |||
There was a problem hiding this comment.
Incomplete migration: DesignToken still used on line 137.
The DesignToken.padding.largeIncreased reference on line 137 was not migrated to KptTheme.spacing.*, leaving a stale DesignToken import on line 45. This should be replaced with the appropriate KptTheme.spacing token to complete the migration.
Proposed fix
-import com.mifos.core.designsystem.theme.DesignToken- verticalArrangement = Arrangement.spacedBy(DesignToken.padding.largeIncreased),
+ verticalArrangement = Arrangement.spacedBy(KptTheme.spacing.md),Also applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt`
at line 45, Replace the stale DesignToken usage and import: locate the reference
to DesignToken.padding.largeIncreased in InterestPage (around the UI layout at
line ~137) and change it to the appropriate KptTheme.spacing token (e.g.,
KptTheme.spacing.largeIncreased or the nearest matching spacing token used
across the module). Update usages in the InterestPage composable/layout and
remove the unused import com.mifos.core.designsystem.theme.DesignToken. Ensure
the new KptTheme.spacing.* token matches surrounding spacing conventions so the
visual layout remains consistent.
There was a problem hiding this comment.
This is intentional
There was a problem hiding this comment.
@kartikey004, understood! I've noted that the DesignToken.padding.largeIncreased usage on line 137 in the RateChart bottom sheet is intentional for this file, consistent with the architectural decision to use both design systems where needed.
🧠 Learnings used
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt:42-42
Timestamp: 2026-02-17T14:39:37.695Z
Learning: In PR `#2610` (KptTheme migration), the PreviewPage.kt file in createShareAccount intentionally retains `DesignToken.spacing.largeIncreased` (20.dp) for the verticalArrangement spacing at line 62 within the scrollable Column, even though other spacing tokens in the file were migrated to KptTheme. This is a deliberate choice to preserve the specific 20.dp spacing between content sections, as KptTheme does not offer an exact 20.dp equivalent.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt:56-61
Timestamp: 2026-02-16T09:59:35.731Z
Learning: In PR `#2610` (KptTheme migration), the PreviewPage.kt outer Column bottom padding was intentionally reduced from DesignToken.padding.large to KptTheme.spacing.sm (Line 57) as a deliberate UI improvement to tighten the layout, not as a direct token-for-token replacement. This spacing reduction improves the overall visual design of the share account preview screen.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt:301-309
Timestamp: 2026-02-17T14:39:16.803Z
Learning: In PR `#2610` (KptTheme migration) for the android-client project, the codebase intentionally uses both DesignToken and KptTheme design systems together. Mixed usage of DesignToken tokens (e.g., DesignToken.padding.medium, DesignToken.sizes.iconMinyMiny) alongside KptTheme tokens (e.g., KptTheme.spacing.md, KptTheme.colorScheme.*) is an intentional architectural decision and should not be flagged as inconsistent migration.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt:98-109
Timestamp: 2026-02-16T08:25:10.251Z
Learning: In the DesignToken.kt file for the android-client project, the team prefers to use raw dp-based naming (dp1, dp2, dp30, dp40, dp44, etc.) for spacing tokens rather than semantic names. This naming convention should be accepted in design token definitions.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt:328-337
Timestamp: 2026-02-16T08:26:24.063Z
Learning: In the DesignToken.kt file for the android-client project, the team prefers to use raw dp-based naming (dp5, dp18, dp20, dp42, dp48, dp72, dp100, dp120, dp128, etc.) for size tokens in the AppSizes data class rather than only semantic names. This naming convention should be accepted in design token definitions.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt:286-289
Timestamp: 2026-02-16T09:01:46.256Z
Learning: In the android-client project PR `#2610` (KptTheme migration), the typography hierarchy from MifosTypography has been simplified when migrating to KptTheme. The `titleMediumEmphasized` style (FontWeight.SemiBold/600) maps to `KptTheme.typography.titleMedium` (FontWeight.Medium/500), representing an intentional design decision to reduce visual emphasis across dialog headers and similar UI elements.
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt:60-60
Timestamp: 2026-02-16T08:37:36.480Z
Learning: In the android-client project, the correct import for KptTheme is `import template.core.base.designsystem.theme.KptTheme`. This is the established import path for the KptTheme design system used throughout the codebase when replacing MaterialTheme references.
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
41ecdad to
48a8700
Compare
|
2 participants
Fixes - Jira-#603
Screenshots (Updated on 17-02-2026):
Summary by CodeRabbit
New Features
Refactor
User-facing polish