MIFOSAC-618 Added toolbar in Navigation Drawer items sections #2567
Conversation
📝 WalkthroughWalkthroughMifosScaffold now conditionally renders a new MifosDetailTopBar component when a title is provided. This top bar includes back navigation, ellipsis-truncated title text, and customizable actions with styling via the design system's TopAppBarDefaults and AppColors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 3
📜 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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosScaffold.kt (1)
23-24: LGTM! Imports are appropriate.The new imports correctly support the TopAppBar implementation including navigation, text styling, and icon resources.
Also applies to: 30-32, 41-42
| topBar = { | ||
| if (title != null) { | ||
| MifosDetailTopBar( | ||
| title = title, | ||
| onBackPressed = onBackPressed, | ||
| actions = actions, | ||
| ) | ||
| } | ||
| }, |
There was a problem hiding this comment.
Default empty onBackPressed creates non-functional back button.
When title is provided but onBackPressed retains its default empty lambda (line 48), the back button appears in the TopAppBar but does nothing when clicked. This violates user expectations and creates a confusing UX.
Consider one of these solutions:
- Provide a functional default for
onBackPressed(e.g., navigation back stack pop) - Make
onBackPressedrequired whentitleis non-null - Conditionally hide the navigation icon when
onBackPressedis not meaningful
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| private fun MifosDetailTopBar( | ||
| title: String, | ||
| onBackPressed: (() -> Unit)?, | ||
| actions: @Composable RowScope.() -> Unit, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify the nullable parameter type.
The onBackPressed parameter is declared as nullable (() -> Unit)?, but the call site at line 63 always passes a non-nullable () -> Unit (with default value {} from line 48). This type inconsistency creates confusion and makes the safe call at line 181 redundant.
🔎 Proposed fix to use non-nullable parameter type
private fun MifosDetailTopBar(
title: String,
- onBackPressed: (() -> Unit)?,
+ onBackPressed: () -> Unit,
actions: @Composable RowScope.() -> Unit,
) {Then update line 181 to remove the safe call:
- IconButton(onClick = { onBackPressed?.invoke() }) {
+ IconButton(onClick = onBackPressed) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosScaffold.kt
around lines 164–169, the onBackPressed parameter is declared nullable (() ->
Unit)? but all callers provide a non-null () -> Unit (default {}), so change the
parameter to a non-nullable () -> Unit, update its default if present, and
remove the redundant safe-call at line 181 (call it directly). Ensure any other
internal references treat it as non-nullable and adjust any call sites that
incorrectly pass null.
| Icon( | ||
| imageVector = MifosIcons.ArrowBack, | ||
| contentDescription = "Navigate back", | ||
| ) |
There was a problem hiding this comment.
Hardcoded contentDescription needs internationalization.
The content description "Navigate back" is hardcoded in English, which prevents proper localization for non-English users and impacts accessibility.
Consider extracting this to a string resource to support i18n/l10n requirements.
🤖 Prompt for AI Agents
In
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosScaffold.kt
around lines 182 to 185, the Icon contentDescription is hardcoded as "Navigate
back"; extract that text into a string resource and use the compose string
retrieval instead of the literal. Add a localized entry (e.g., navigate_back) in
the appropriate resources (strings.xml / common resource file), import and call
stringResource(R.string.navigate_back) (or the multiplatform/localization helper
your project uses) for the contentDescription, and ensure the new resource key
is used consistently so the description is localizable.
4 participants
Fixes - Jira-#618
Updated logic to conditionally render a TopAppBar when a title is provided. Previously, the topBar parameter was explicitly set to empty, hiding it for all detail screens.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.