refactor(feature): replace MaterialTheme with kptTheme#2610
refactor(feature): replace MaterialTheme with kptTheme#2610biplab1 merged 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
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientStaff/ClientStaffScreen.kt
Show resolved
Hide resolved
...loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeDialog/LoanChargeDialogScreen.kt
Outdated
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt
Show resolved
Hide resolved
...in/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt
Show resolved
Hide resolved
...ure/report/src/commonMain/kotlin/com/mifos/feature/report/reportDetail/ReportDetailScreen.kt
Outdated
Show resolved
Hide resolved
...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
Outdated
Show resolved
Hide resolved
...lient/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt
Outdated
Show resolved
Hide resolved
...e/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt
Show resolved
Hide resolved
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.
feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt
Outdated
Show resolved
Hide resolved
feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
...e/center/src/commonMain/kotlin/com/mifos/feature/center/centerDetails/CenterDetailsScreen.kt
Outdated
Show resolved
Hide resolved
...e/center/src/commonMain/kotlin/com/mifos/feature/center/centerDetails/CenterDetailsScreen.kt
Outdated
Show resolved
Hide resolved
...ure/center/src/commonMain/kotlin/com/mifos/feature/center/centerGroupList/GroupListScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/center/syncCentersDialog/SyncCentersDialogScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/center/syncCentersDialog/SyncCentersDialogScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/center/syncCentersDialog/SyncCentersDialogScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInbox/CheckerInboxScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInbox/CheckerInboxScreen.kt
Outdated
Show resolved
Hide resolved
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.
...lient/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Outdated
Show resolved
Hide resolved
.../offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt
Outdated
Show resolved
Hide resolved
...n/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt
Show resolved
Hide resolved
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
41ecdad to
48a8700
Compare
.../center/src/commonMain/kotlin/com/mifos/feature/center/createCenter/CreateNewCenterScreen.kt
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/center/syncCentersDialog/SyncCentersDialogScreen.kt
Outdated
Show resolved
Hide resolved
...lin/com/mifos/feature/checker/inbox/task/checkerInboxDialog/CheckerInboxTasksFilterDialog.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientAddDocuments/ClientAddDocumentsScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientAddDocuments/ClientAddDocumentsScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/client/clientAddress/addAddress/AddAddressScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/client/clientAddress/addAddress/AddAddressScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/client/clientAddress/addAddress/AddAddressScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/client/clientAddress/addAddress/AddAddressScreen.kt
Outdated
Show resolved
Hide resolved
...e/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/ClientAddressScreen.kt
Outdated
Show resolved
Hide resolved
|
@kartikey004 Please take a look at this table and modify accordingly, openMF/mifos-mobile#3013 (comment) |
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
5 times, most recently
from
fdf875b to
d3bb95a
Compare
amanna13
left a comment
amanna13
left a comment
There was a problem hiding this comment.
@kartikey004 Please align the DesignToken with reference to my pr - https://github.com/openMF/android-client/pull/2615/changes#diff-16895a86edd904a31eac7b1c21260de24bafc92ef520683d1973c217aba3a692R350
I have taken ref. from the already exiting changes in the mifos-mobile and also added few new which were not present earlier.
I have already reviewed and aligned the files in the feature module accordingly. Currently waiting for Biplab sir to review and share any further suggestions. |
I don't think so. I'm talking about the |
...rc/commonMain/kotlin/com/mifos/feature/client/clientAddDocuments/ClientAddDocumentsScreen.kt
Outdated
Show resolved
Hide resolved
...e/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/ClientAddressScreen.kt
Outdated
Show resolved
Hide resolved
amanna13
left a comment
amanna13
left a comment
There was a problem hiding this comment.
Plese update with the changes requested. Thanks!
...otlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/documentPreviewScreen/DocumentPreviewScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/documentPreviewScreen/DocumentPreviewScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/documentPreviewScreen/DocumentPreviewScreen.kt
Outdated
Show resolved
Hide resolved
...ient/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt
Outdated
Show resolved
Hide resolved
feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt
Outdated
Show resolved
Hide resolved
...lin/com/mifos/feature/checker/inbox/task/checkerInboxDialog/CheckerInboxTasksFilterDialog.kt
Outdated
Show resolved
Hide resolved
.../savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/TermsPage.kt
Show resolved
Hide resolved
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
b7a0e3c to
2297f2d
Compare
|
@kartikey004 Please fix the static checks. |
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
2 times, most recently
from
cb37305 to
ec367ff
Compare
.../src/commonMain/kotlin/com/mifos/feature/center/syncCentersDialog/SyncCentersDialogScreen.kt
Outdated
Show resolved
Hide resolved
...Main/kotlin/com/mifos/feature/client/clientDetailsProfile/components/ClientDetailsProfile.kt
Show resolved
Hide resolved
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt
Outdated
Show resolved
Hide resolved
...e/client/src/commonMain/kotlin/com/mifos/feature/client/clientSurveyList/SurveyListScreen.kt
Outdated
Show resolved
Hide resolved
...ient/src/commonMain/kotlin/com/mifos/feature/client/clientSurveySubmit/SurveySubmitScreen.kt
Outdated
Show resolved
Hide resolved
...ient/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt
Outdated
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
ec367ff to
1b20be2
Compare
|
@kartikey004 Please wait for |
kartikey004
force-pushed
the
refactor/kpt-theme-feat
branch
from
1b20be2 to
dfd23a5
Compare
|
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
3 participants
Fixes - Jira-#603
Screenshots (Updated on 17-02-2026):
Summary by CodeRabbit
New Features
Refactor
User-facing polish