Added toolbar for Navigation Drawer screens #2566
Conversation
…ive and does not submit the form
📝 WalkthroughWalkthroughIntroduces Material3 TopAppBar usage across many screens, refactors Client list UI, rewrites Payment Details screen with scaffold/top bar and additional fields, threads onBackPressed through collection sheet navigation, replaces some Scaffolds with scrolling Columns, and updates one string resource. Changes
Sequence Diagram(s)sequenceDiagram
%%{init: {"themeVariables":{"actorBackground":"#F6F7FB","actorBorder":"#D0D7E6","noteBackground":"#F2F4F8"}}}%%
participant Nav as NavHost
participant NavBuilder as CollectionSheetNavGraph
participant Screen as PaymentDetailsScreenRoute
participant UI as PaymentsDetailsScreen
participant VM as PaymentDetailsViewModel
participant Snackbar as SnackbarHost
Nav ->> NavBuilder: navigateToPaymentDetailsScreen(payload)
NavBuilder ->> Screen: paymentDetailsScreen(onBackPressed)
Screen ->> UI: composable(payload, onBackPressed)
UI ->> VM: loadPaymentData(clientId)
VM -->> UI: paymentData
UI ->> UI: user edits fields, toggles payment type
UI ->> VM: onSaveAdditionalItem(payload)
VM -->> Snackbar: emit success/failure
UI ->> Snackbar: show message
UI ->> Nav: onBackPressed() --x back navigation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/newIndividualCollectionSheet/NewIndividualCollectionSheetScreen.kt (1)
151-213: Remove nested scroll modifiers to fix scrolling conflicts.The outer Column (line 151) and the inner Column (line 210) both have
verticalScroll(rememberScrollState()), creating nested scrollable containers in the same direction. This will cause scrolling conflicts and unpredictable behavior where touch events cannot determine which scrollable should respond.🔎 Proposed fix
Remove the scroll modifier from the outer Column since the inner Column (which contains the actual content) should handle scrolling:
Column( modifier = modifier - .fillMaxSize() - .verticalScroll(rememberScrollState()), + .fillMaxSize(), ) {feature/report/src/commonMain/kotlin/com/mifos/feature/report/runReport/RunReportScreen.kt (1)
165-165: Use theme color instead of hardcoded White.The DropdownMenu background uses a hardcoded
Whitecolor, which won't adapt to theme changes (e.g., dark mode) and breaks theme consistency.🔎 Proposed fix
DropdownMenu( - modifier = Modifier.background(White), + modifier = Modifier.background(MaterialTheme.colorScheme.surface), expanded = showMenu, onDismissRequest = { showMenu = false }, ) {feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/paymentDetails/PaymentDetailsScreen.kt (1)
153-198: Validation flagisAnyDetailNullis computed but not enforced.The
saveAdditionalfunction setsisAnyDetailNull = truewhen fields are empty, but then saves the transaction regardless. This allows incomplete payment details to be saved without user feedback.Consider either:
- Preventing the save operation when
isAnyDetailNullis true and showing a snackbar/error message to the user- Removing the validation logic if partial details are acceptable
🔎 Proposed fix to enforce validation
if (!isAnyDetailNull) { noPaymentVisibility = false + onSaveAdditionalItem(bulkRepaymentTransactions, position) + showAdditionalDetails = false + } else { + // Show error to user via snackbar } - onSaveAdditionalItem(bulkRepaymentTransactions, position) - showAdditionalDetails = false }
🧹 Nitpick comments (10)
feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt (1)
74-75: Consider localizing the contentDescription string.The hardcoded string "Navigate back" should ideally be extracted to a string resource for proper localization support, consistent with other UI strings in the file.
💡 Example refactor
Add a string resource (e.g.,
feature_about_navigate_back) and use:- contentDescription = "Navigate back" + contentDescription = stringResource(Res.string.feature_about_navigate_back)feature/path-tracking/src/commonMain/kotlin/com/mifos/feature/path/tracking/PathTrackingScreen.kt (1)
132-136: Use string resource for contentDescription.The hardcoded string "Navigate back" should be externalized to a string resource for proper localization support.
🔎 Suggested approach
Add a string resource (e.g.,
feature_path_tracking_navigate_back) and reference it:navigationIcon = { IconButton(onClick = onBackPressed) { Icon( imageVector = MifosIcons.ArrowBack, - contentDescription = "Navigate back" + contentDescription = stringResource(Res.string.feature_path_tracking_navigate_back) ) } },feature/report/src/commonMain/kotlin/com/mifos/feature/report/runReport/RunReportScreen.kt (1)
149-157: Use MaterialTheme typography instead of manual TextStyle.The menu title Text uses a manually constructed TextStyle rather than MaterialTheme.typography, which reduces consistency with the app's design system.
🔎 Proposed fix
Text( text = menuTitle, - style = TextStyle( - fontSize = 24.sp, - fontWeight = FontWeight.Medium, - fontStyle = FontStyle.Normal, - ), + style = MaterialTheme.typography.titleLarge, textAlign = TextAlign.Start, )feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt (1)
135-140: Use string resource for contentDescription.The hardcoded string "Navigate back" should be externalized to a string resource for proper localization support.
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/settings/SettingsScreen.kt (1)
124-129: Use string resource for contentDescription.The hardcoded string "Navigate back" should be externalized to a string resource for proper localization support and consistency with other accessibility strings.
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/individualCollectionSheet/IndividualCollectionSheetScreen.kt (1)
72-76: Use string resource for contentDescription.The hardcoded string "Navigate back" should be externalized to a string resource for proper localization support.
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/paymentDetails/PaymentDetailsScreen.kt (2)
216-216: Use string resource for the title.The hardcoded "Payment Details" string should be externalized to a string resource for proper localization support.
🔎 Suggested approach
Add a string resource (e.g.,
feature_collection_sheet_payment_details) and reference it:TopAppBar( - title = { Text(text = "Payment Details") }, + title = { Text(text = stringResource(Res.string.feature_collection_sheet_payment_details)) }, navigationIcon = {
218-223: Use string resource for contentDescription.The hardcoded string "Navigate back" should be externalized to a string resource for proper localization support.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (2)
319-321: Consider using Scaffold's contentPadding instead of hardcoded bottom padding.The hardcoded 80.dp bottom padding likely accounts for the FAB, but this approach is fragile. The Scaffold already provides
contentPaddingthrough its lambda parameter, which automatically handles insets for FABs, bottom bars, etc. Consider using that instead for better adaptability across different screen configurations.🔎 Suggested improvement
@Composable -private fun LazyColumnForClientListDb(clientList: List<ClientEntity>) { +private fun LazyColumnForClientListDb( + clientList: List<ClientEntity>, + contentPadding: PaddingValues = PaddingValues(0.dp) +) { LazyColumn( - contentPadding = PaddingValues(top = 8.dp, bottom = 80.dp) + contentPadding = PaddingValues( + top = 8.dp, + bottom = contentPadding.calculateBottomPadding() + ) ) {Then pass the scaffold's padding from the call site.
430-432: Column wrapper in preview appears unnecessary.The
Columnwrapper aroundLazyColumnForClientListDbin the preview doesn't appear to serve a purpose. If it's not needed for future additions, consider removing it for simplicity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.ktfeature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInboxTasks/CheckerInboxTasksScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.ktfeature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/individualCollectionSheet/IndividualCollectionSheetScreen.ktfeature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/navigation/CollectionSheetNavigation.ktfeature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/newIndividualCollectionSheet/NewIndividualCollectionSheetScreen.ktfeature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/paymentDetails/PaymentDetailsScreen.ktfeature/offline/src/commonMain/composeResources/values/strings.xmlfeature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.ktfeature/path-tracking/src/commonMain/kotlin/com/mifos/feature/path/tracking/PathTrackingScreen.ktfeature/report/src/commonMain/kotlin/com/mifos/feature/report/runReport/RunReportScreen.ktfeature/settings/src/commonMain/kotlin/com/mifos/feature/settings/settings/SettingsScreen.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/paymentDetails/PaymentDetailsScreen.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosUserImage.kt (1)
MifosUserImage(31-68)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosButton.kt (2)
MifosButton(52-78)MifosButton(91-117)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/navigation/CollectionSheetNavigation.kt (1)
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/paymentDetails/PaymentDetailsScreen.kt (1)
PaymentDetailsScreenRoute(76-93)
🪛 GitHub Actions: PR Checks
feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt
[error] 65-65: Spotless Kotlin formatting violations. Run './gradlew :feature:about:spotlessApply' to fix these violations.
🔇 Additional comments (9)
feature/offline/src/commonMain/composeResources/values/strings.xml (1)
13-13: LGTM! String cleanup aligns with the new TopAppBar pattern.The removal of the asterisk from "Offline Sync *" to "Offline Sync" is appropriate for the consistent toolbar presentation introduced in this PR.
feature/checker-inbox-task/src/commonMain/kotlin/com/mifos/feature/checker/inbox/task/checkerInboxTasks/CheckerInboxTasksScreen.kt (1)
170-172: Verify the TaskOptions card transparency change.The
TaskOptionscard'scontainerColoris now set toColor.Transparent. This styling change is not mentioned in the PR description, which focuses on adding toolbar navigation.Please confirm whether this visual change is intentional and related to the toolbar addition, or if it should be documented separately.
feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt (4)
30-30: LGTM: Imports are correct.The new imports are necessary for the TopAppBar implementation and follow the appropriate Material3 component structure.
Also applies to: 32-32, 36-37, 45-45
63-85: TopAppBar implementation achieves PR objectives.The TopAppBar structure correctly implements the navigation drawer screen toolbar pattern, providing:
- Consistent title display
- Back navigation via IconButton
- Proper Material3 theming
This aligns with the PR goal of adding toolbars to Navigation Drawer destination screens for improved navigation.
72-77:MifosIcons.ArrowBackis defined and available in the design system. It maps toIcons.Filled.ChevronLeftat line 198 ofcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktand is used consistently across multiple screens in the codebase.
54-54: Remove the @OptIn(ExperimentalMaterial3Api::class) annotation or verify it's only needed for other experimental APIs in this file.TopAppBar is stable in Jetpack Compose Material3 (as of 1.5.0-alpha11). The experimental APIs were promoted to stable in recent releases, making the OptIn annotation unnecessary for TopAppBar specifically. If this annotation is used for other experimental Material3 APIs in this file, keep it and add a comment clarifying which other APIs require it.
Likely an incorrect or invalid review comment.
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/navigation/CollectionSheetNavigation.kt (1)
53-53: LGTM: Back navigation properly threaded through the navigation graph.The onBackPressed parameter is correctly propagated from the navigation graph to the paymentDetailsScreen function and into PaymentDetailsScreenRoute, enabling proper back navigation handling.
Also applies to: 104-116
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (2)
1-433: Verify that these UI redesign changes belong in this PR.The PR objectives describe adding toolbars to Navigation Drawer screens for back navigation, but this file contains client list UI redesign changes (card styling, avatar rendering, text formatting). These appear unrelated to the toolbar additions described in the PR objectives. Mixing unrelated changes in a single PR can complicate review, testing, and rollback if issues arise.
165-165: Verify the containerColor change is intentional.The scaffold's
containerColorwas changed fromsurfaceContainertosurface. Ensure this change is intentional and maintains visual consistency with other screens in the app.
| // title = stringResource(Res.string.feature_about), | ||
| // onBackPressed = onBackPressed, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
Dead code should be deleted rather than commented out. Version control preserves the history if needed.
🔎 Proposed fix
-// title = stringResource(Res.string.feature_about),
-// onBackPressed = onBackPressed,
snackbarHostState = remember { SnackbarHostState() },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // title = stringResource(Res.string.feature_about), | |
| // onBackPressed = onBackPressed, | |
| snackbarHostState = remember { SnackbarHostState() }, |
🤖 Prompt for AI Agents
In feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt
around lines 60 to 61, there are two commented-out lines (title =
stringResource(Res.string.feature_about), onBackPressed = onBackPressed,) that
should be removed; just delete those commented lines so the file contains no
dead/commented-out code and keep surrounding indentation/formatting intact.
| topBar = { | ||
| TopAppBar( | ||
| title = { | ||
| Text( | ||
| text = stringResource(Res.string.feature_about), | ||
| style = MaterialTheme.typography.titleLarge | ||
| ) | ||
| }, | ||
| navigationIcon = { | ||
| IconButton(onClick = onBackPressed) { | ||
| Icon( | ||
| imageVector = MifosIcons.ArrowBack, | ||
| contentDescription = "Navigate back" | ||
| ) | ||
| } | ||
| }, | ||
| colors = TopAppBarDefaults.topAppBarColors( | ||
| containerColor = MaterialTheme.colorScheme.surface, | ||
| titleContentColor = MaterialTheme.colorScheme.onSurface, | ||
| navigationIconContentColor = MaterialTheme.colorScheme.onSurface | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix Spotless formatting violations before merge.
The pipeline indicates a formatting violation at line 65. Run the following command to auto-fix:
./gradlew :feature:about:spotlessApply🧰 Tools
🪛 GitHub Actions: PR Checks
[error] 65-65: Spotless Kotlin formatting violations. Run './gradlew :feature:about:spotlessApply' to fix these violations.
🤖 Prompt for AI Agents
In feature/about/src/commonMain/kotlin/com/mifos/feature/about/AboutScreen.kt
around lines 63 to 85 the file fails Spotless formatting (violation reported at
line 65); run the auto-formatter to fix formatting by executing ./gradlew
:feature:about:spotlessApply, verify formatting with ./gradlew
:feature:about:spotlessCheck (or CI), then stage and commit the changed file and
push the branch so the PR passes the Spotless check.
| topBar = { | ||
| TopAppBar( | ||
| navigationIcon = { | ||
| IconButton(onClick = onBackPressed) { | ||
| Icon( | ||
| imageVector = MifosIcons.ArrowBack, | ||
| contentDescription = null, | ||
| ) | ||
| } | ||
| }, | ||
| title = { | ||
| Text( | ||
| text = stringResource( | ||
| Res.string.feature_checker_inbox_task_checker_Inbox | ||
| ) | ||
| ) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Provide accessible label for back navigation button.
The back button's contentDescription is set to null, which prevents screen reader users from understanding the button's purpose.
🔎 Proposed fix
navigationIcon = {
IconButton(onClick = onBackPressed) {
Icon(
imageVector = MifosIcons.ArrowBack,
- contentDescription = null,
+ contentDescription = "Navigate back",
)
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| topBar = { | |
| TopAppBar( | |
| navigationIcon = { | |
| IconButton(onClick = onBackPressed) { | |
| Icon( | |
| imageVector = MifosIcons.ArrowBack, | |
| contentDescription = null, | |
| ) | |
| } | |
| }, | |
| title = { | |
| Text( | |
| text = stringResource( | |
| Res.string.feature_checker_inbox_task_checker_Inbox | |
| ) | |
| ) | |
| }, | |
| ) | |
| } | |
| topBar = { | |
| TopAppBar( | |
| navigationIcon = { | |
| IconButton(onClick = onBackPressed) { | |
| Icon( | |
| imageVector = MifosIcons.ArrowBack, | |
| contentDescription = "Navigate back", | |
| ) | |
| } | |
| }, | |
| title = { | |
| Text( | |
| text = stringResource( | |
| Res.string.feature_checker_inbox_task_checker_Inbox | |
| ) | |
| ) | |
| }, | |
| ) | |
| } |
| Surface( | ||
| shape = CircleShape, | ||
| color = MaterialTheme.colorScheme.primaryContainer, | ||
| modifier = Modifier.size(48.dp) | ||
| ) { | ||
| Box(contentAlignment = Alignment.Center) { | ||
| Image( | ||
| painter = painterResource(Res.drawable.feature_client_ic_dp_placeholder), | ||
| contentDescription = null, | ||
| modifier = Modifier | ||
| .size(28.dp) | ||
| .fillMaxSize(), | ||
| contentScale = ContentScale.Fit, | ||
| colorFilter = ColorFilter.tint(MaterialTheme.colorScheme.onPrimaryContainer) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant fillMaxSize() modifier.
Line 351 applies fillMaxSize() to an Image that already has a fixed size(28.dp) modifier on line 350. The fillMaxSize() is redundant and contradicts the fixed size. Remove it for clarity.
🔎 Proposed fix
Image(
painter = painterResource(Res.drawable.feature_client_ic_dp_placeholder),
contentDescription = null,
modifier = Modifier
- .size(28.dp)
- .fillMaxSize(),
+ .size(28.dp),
contentScale = ContentScale.Fit,
colorFilter = ColorFilter.tint(MaterialTheme.colorScheme.onPrimaryContainer)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Surface( | |
| shape = CircleShape, | |
| color = MaterialTheme.colorScheme.primaryContainer, | |
| modifier = Modifier.size(48.dp) | |
| ) { | |
| Box(contentAlignment = Alignment.Center) { | |
| Image( | |
| painter = painterResource(Res.drawable.feature_client_ic_dp_placeholder), | |
| contentDescription = null, | |
| modifier = Modifier | |
| .size(28.dp) | |
| .fillMaxSize(), | |
| contentScale = ContentScale.Fit, | |
| colorFilter = ColorFilter.tint(MaterialTheme.colorScheme.onPrimaryContainer) | |
| ) | |
| } | |
| } | |
| Surface( | |
| shape = CircleShape, | |
| color = MaterialTheme.colorScheme.primaryContainer, | |
| modifier = Modifier.size(48.dp) | |
| ) { | |
| Box(contentAlignment = Alignment.Center) { | |
| Image( | |
| painter = painterResource(Res.drawable.feature_client_ic_dp_placeholder), | |
| contentDescription = null, | |
| modifier = Modifier | |
| .size(28.dp), | |
| contentScale = ContentScale.Fit, | |
| colorFilter = ColorFilter.tint(MaterialTheme.colorScheme.onPrimaryContainer) | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt
around lines 340 to 356, the Image has a redundant fillMaxSize() modifier
applied after size(28.dp) which contradicts the fixed size; remove the
fillMaxSize() call so the Image only uses size(28.dp) (and any other needed
modifiers) to enforce the intended fixed dimensions and avoid conflicting layout
behavior.
| Spacer(modifier = Modifier.height(4.dp)) | ||
| Text( | ||
| text = client.accountNo.toString(), | ||
| text = "Account: ${client.accountNo}", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for string resource files
fd -t f -e xml | grep -E "(strings|resources)" | head -20Repository: openMF/android-client
Length of output: 1202
🏁 Script executed:
# Check the project structure to understand localization setup
ls -la | head -20
find . -type d -name "values*" | head -10Repository: openMF/android-client
Length of output: 1620
🏁 Script executed:
# Examine the ClientListScreen file to see how other UI text is handled
head -100 feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.ktRepository: openMF/android-client
Length of output: 4528
🏁 Script executed:
# Search for similar patterns in the same file - how are other text strings handled
rg "text\s*=" feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt -A 1 -B 1 | head -40Repository: openMF/android-client
Length of output: 536
🏁 Script executed:
# Check the feature/client strings.xml file to see existing localized strings
cat feature/client/src/commonMain/composeResources/values/strings.xmlRepository: openMF/android-client
Length of output: 44171
🏁 Script executed:
# Search for "Account" related strings in the project to see if similar strings are localized
rg -i "account" --type xml feature/client/src/ | head -20Repository: openMF/android-client
Length of output: 3123
🏁 Script executed:
# Check how other similar labels/prefixes are handled in ClientListScreen context
rg -B 5 -A 2 "text = \"Account" feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.ktRepository: openMF/android-client
Length of output: 500
Extract the "Account: " prefix to a string resource for localization.
The hardcoded text at line 375 should use stringResource() to support the project's localization framework. An existing string resource feature_client_account_number ("Account Number") is available in the feature module's strings.xml, or a new resource like feature_client_account_label can be created for the "Account: " prefix to align with the app's multi-language support (Catalan, English, Spanish, French, Hindi, Kannada, Swahili, Chinese).
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt
around line 375, the hardcoded "Account: " prefix should be moved to a string
resource and used via stringResource() for localization; add a new string
resource (e.g., feature_client_account_label with value "Account: %s") or reuse
feature_client_account_number appropriately, then replace the hardcoded literal
with stringResource(R.string.feature_client_account_label, client.accountNo)
(ensure androidx.compose.ui.res.stringResource is imported) so the prefix is
localized and the account number injected via formatting.
| OutlinedCard( | ||
| modifier = Modifier.padding(horizontal = 16.dp), | ||
| colors = CardDefaults.cardColors( | ||
| containerColor = Color.White, | ||
| ), | ||
| ) { |
There was a problem hiding this comment.
Use theme color instead of hardcoded Color.White.
The OutlinedCard uses a hardcoded Color.White for the container color, which won't adapt to dark mode and breaks theme consistency.
🔎 Proposed fix
OutlinedCard(
modifier = Modifier.padding(horizontal = 16.dp),
colors = CardDefaults.cardColors(
- containerColor = Color.White,
+ containerColor = MaterialTheme.colorScheme.surface,
),
) {🤖 Prompt for AI Agents
In
feature/collectionSheet/src/commonMain/kotlin/com/mifos/feature/individualCollectionSheet/paymentDetails/PaymentDetailsScreen.kt
around lines 326-331, the OutlinedCard uses a hardcoded Color.White for
containerColor which breaks theme/dark-mode; replace the hardcoded Color.White
with the theme surface/background color (e.g., MaterialTheme.colorScheme.surface
or MaterialTheme.colorScheme.background depending on your design system) by
updating the CardDefaults.cardColors call to use MaterialTheme.colorScheme.* so
the card adapts to dark mode and the app theme.
| // onBackPressed = onBackPressed, | ||
| // title = stringResource(Res.string.feature_offline_offline_Sync), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The commented scaffold parameters should be removed as they're now handled by the TopAppBar.
🔎 Proposed fix
)
)
}
-// onBackPressed = onBackPressed,
-// title = stringResource(Res.string.feature_offline_offline_Sync),
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // onBackPressed = onBackPressed, | |
| // title = stringResource(Res.string.feature_offline_offline_Sync), | |
| ) | |
| ) | |
| } | |
| ) { |
🤖 Prompt for AI Agents
In
feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/dashboard/OfflineDashboardScreen.kt
around lines 149 to 150, remove the two commented-out scaffold parameter lines
("onBackPressed = onBackPressed," and "title =
stringResource(Res.string.feature_offline_offline_Sync),") since the TopAppBar
now handles these; simply delete those commented lines so the file contains no
dead commented scaffold parameters.
| // title = stringResource(Res.string.feature_path_tracking_track_my_path), | ||
| // onBackPressed = onBackPressed, | ||
| // actions = { | ||
| // IconButton( | ||
| // onClick = { | ||
| // if (userStatus) { | ||
| // updateUserStatus(false) | ||
| // } else { | ||
| // checkPermission = true | ||
| // } | ||
| // }, | ||
| // ) { | ||
| // Icon( | ||
| // imageVector = if (userStatus) MifosIcons.Stop else MifosIcons.MyLocation, | ||
| // contentDescription = null, | ||
| // ) | ||
| // } | ||
| // }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The commented scaffold configuration (lines 104-121) should be removed rather than left in the codebase. Commented code creates maintenance debt and can confuse future developers.
🔎 Proposed fix
MifosScaffold(
modifier = modifier,
-// title = stringResource(Res.string.feature_path_tracking_track_my_path),
-// onBackPressed = onBackPressed,
-// actions = {
-// IconButton(
-// onClick = {
-// if (userStatus) {
-// updateUserStatus(false)
-// } else {
-// checkPermission = true
-// }
-// },
-// ) {
-// Icon(
-// imageVector = if (userStatus) MifosIcons.Stop else MifosIcons.MyLocation,
-// contentDescription = null,
-// )
-// }
-// },
snackbarHostState = snackbarHostState,
topBar = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // title = stringResource(Res.string.feature_path_tracking_track_my_path), | |
| // onBackPressed = onBackPressed, | |
| // actions = { | |
| // IconButton( | |
| // onClick = { | |
| // if (userStatus) { | |
| // updateUserStatus(false) | |
| // } else { | |
| // checkPermission = true | |
| // } | |
| // }, | |
| // ) { | |
| // Icon( | |
| // imageVector = if (userStatus) MifosIcons.Stop else MifosIcons.MyLocation, | |
| // contentDescription = null, | |
| // ) | |
| // } | |
| // }, |
🤖 Prompt for AI Agents
In
feature/path-tracking/src/commonMain/kotlin/com/mifos/feature/path/tracking/PathTrackingScreen.kt
around lines 104 to 121 the scaffold configuration is commented out; remove
these commented lines entirely to eliminate dead code and reduce maintenance
debt, ensuring no functional changes are made and the file compiles cleanly
after deletion.
There was a problem hiding this comment.
MifosScaffold(
title = stringResource(Res.string.feature_collection_sheet_individual_collection_sheet),
onBackPressed = onBackPressed,
)
don't remove above one
internally to Mifos Scaffold we using top bar see why it not showing and do modifications to Mifos scaffold no need of changing in every screen
Sure. I have made the changes. I'll create a fresh PR after passing the checks. Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosScaffold.kt
🔇 Additional comments (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosScaffold.kt (2)
23-24: LGTM! Necessary imports for the new top bar functionality.The added imports support the new conditional top bar feature and follow standard Material3 patterns.
Also applies to: 30-32, 41-42
59-67: Well done! Conditional top bar rendering provides backward compatibility.The implementation correctly renders the top bar only when a title is provided, maintaining backward compatibility with existing screens that don't use the title parameter.
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| private fun MifosDetailTopBar( | ||
| title: String, | ||
| onBackPressed: (() -> Unit)?, | ||
| actions: @Composable RowScope.() -> Unit, | ||
| ) { | ||
| TopAppBar( | ||
| title = { | ||
| Text( | ||
| text = title, | ||
| maxLines = 1, | ||
| overflow = TextOverflow.Ellipsis, | ||
| style = MaterialTheme.typography.titleMedium, | ||
| ) | ||
| }, | ||
| navigationIcon = { | ||
| IconButton(onClick = { onBackPressed?.invoke() }) { | ||
| Icon( | ||
| imageVector = MifosIcons.ArrowBack, | ||
| contentDescription = "Navigate back", | ||
| ) | ||
| } | ||
| }, | ||
| actions = actions, | ||
| colors = TopAppBarDefaults.topAppBarColors( | ||
| containerColor = AppColors.customWhite, | ||
| titleContentColor = MaterialTheme.colorScheme.onSurface, | ||
| navigationIconContentColor = MaterialTheme.colorScheme.onSurface, | ||
| actionIconContentColor = MaterialTheme.colorScheme.onSurface, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Good implementation following Material3 patterns, with a minor i18n issue.
The top bar implementation is well-structured and properly styled. However, there's a localization concern.
Issue: Hardcoded content description at line 184
The string "Navigate back" should be externalized to a string resource for proper internationalization support.
🔎 Suggested fix
Assuming you have a string resource available (e.g., Res.string.navigate_back), update the content description:
Icon(
imageVector = MifosIcons.ArrowBack,
- contentDescription = "Navigate back",
+ contentDescription = stringResource(Res.string.navigate_back),
)If the string resource doesn't exist yet, add it to your strings file first.
Optional: Consider simplifying the nullable onBackPressed parameter
The onBackPressed parameter at line 168 is nullable, but it's always provided as non-null from the caller at line 63. You could simplify the signature:
🔎 Optional refactor
@Composable
private fun MifosDetailTopBar(
title: String,
- onBackPressed: (() -> Unit)?,
+ onBackPressed: () -> Unit,
actions: @Composable RowScope.() -> Unit,
) {
TopAppBar(
// ...
navigationIcon = {
- IconButton(onClick = { onBackPressed?.invoke() }) {
+ IconButton(onClick = onBackPressed) {
Icon(
imageVector = MifosIcons.ArrowBack,
contentDescription = "Navigate back",
)
}
},
// ...
)
}🤖 Prompt for AI Agents
In
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosScaffold.kt
around lines 164 to 196, the Icon's contentDescription is hardcoded as "Navigate
back" which breaks i18n; replace the hardcoded string with a string resource
lookup (use stringResource(R.string.navigate_back)) for the contentDescription,
and if the string resource doesn't exist add an entry like "navigate_back" to
your strings file with the appropriate translations; optionally, after this,
consider making onBackPressed non-null if all callers pass it, but the immediate
fix is to externalize the content description to resources.
2 participants
Fixes - Jira-#618
Problem
When a user opens the navigation drawer and selects any drawer item (e.g. Settings, About, Path Tracking, Checker Inbox), the destination screen is opened without a toolbar, leaving no way to navigate back.
Solution
Added a top app bar to all Navigation Drawer destination screens
Toolbar includes a Back button to allow users to return to the previous screen
Ensured consistent behavior across all drawer items
Home / bottom-navigation screens remain unchanged and continue to show the menu icon
Result
All drawer screens now have a visible toolbar
Back navigation works correctly from every drawer destination
Improves usability and navigation consistency
Screenshots
Summary by CodeRabbit
Style
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.