feat(pocket): add pocket linked accounts POC#2020
Open
bhuvan-somisetty wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements the complete Pocket feature, enabling users to view linked financial accounts, link/delink accounts, and access account details through a new dashboard screen. The implementation spans data models, dual repository implementations (demo and API-based), network service definitions, two Compose screens with ViewModels, navigation setup, and home screen integration. ChangesPocket Feature Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/DemoPocketRepository.kt (1)
84-85: ⚡ Quick winConsider logging or throwing on invalid account type.
The
toPocketAccountType()function silently falls back toSAVINGSwhen it encounters an unrecognized account type. During development and testing, this silent fallback could hide bugs (e.g., typos in account type strings).🔍 Suggested improvement
For a POC/demo implementation, consider logging the fallback or throwing an exception to catch issues early:
private fun String.toPocketAccountType(): PocketAccountType = - PocketAccountType.entries.firstOrNull { it.name == uppercase() } ?: PocketAccountType.SAVINGS + PocketAccountType.entries.firstOrNull { it.name == uppercase() } + ?: run { + println("Warning: Unknown account type '$this', defaulting to SAVINGS") + PocketAccountType.SAVINGS + }Or throw an exception:
private fun String.toPocketAccountType(): PocketAccountType = - PocketAccountType.entries.firstOrNull { it.name == uppercase() } ?: PocketAccountType.SAVINGS + PocketAccountType.entries.firstOrNull { it.name == uppercase() } + ?: error("Invalid account type: $this")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/DemoPocketRepository.kt` around lines 84 - 85, The helper toPocketAccountType() silently maps unknown strings to PocketAccountType.SAVINGS which can hide bugs; update to detect when firstOrNull returns null and either (a) log a warning with the original String and the fallback (include the function name toPocketAccountType and the constant PocketAccountType.SAVINGS in the message) or (b) throw an explicit exception (e.g., IllegalArgumentException) with the invalid value so failures are visible during dev/testing—choose logging for leniency or throwing for strict failure, and implement the chosen behavior inside toPocketAccountType().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt`:
- Line 181: The startDestination is currently set to POCKET_DASHBOARD_ROUTE in
MifosNavHost (the assignment of startDestination = POCKET_DASHBOARD_ROUTE),
which makes Pocket Dashboard the app entry point; change this assignment to the
Home route constant (use the HOME_ROUTE / equivalent Home route identifier used
elsewhere in the navigation graph) so the app boots into Home and preserves the
home-first flow and the “home shortcut to Pocket” behavior; update the
startDestination assignment only and verify related navigation graph code in
MifosNavHost and any references to POCKET_DASHBOARD_ROUTE remain unchanged
elsewhere.
- Around line 578-581: The onLinkSuccess callback in the linkAccountScreen
currently calls navController.navigateToPocketDashboard() while navigateBack is
set to navController::navigateUp, causing duplicate Pocket Dashboard entries;
remove the explicit navController.navigateToPocketDashboard() call from the
onLinkSuccess lambda (or replace it with a no-op) so that success handling
relies solely on navigateBack/navController::navigateUp to return to the
dashboard and avoid pushing the dashboard twice.
In
`@core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PocketRepositoryImpl.kt`:
- Line 35: The Flow.catch blocks in PocketRepositoryImpl that currently do
`.catch { emit(DataState.Error(it)) }` must preserve cooperative cancellation:
change each catch handler to rethrow CancellationException (e.g., if (it is
CancellationException) throw it) and only emit DataState.Error for
non-cancellation exceptions; update all three catch sites in
PocketRepositoryImpl (the catch wrapping Flow emissions that emit
DataState.Error) and ensure kotlin.coroutines.cancellation.CancellationException
is referenced/imported so cancellation is not swallowed.
In
`@core/network/src/commonMain/kotlin/org/mifospay/core/network/services/PocketService.kt`:
- Around line 23-24: Rename the service method getPockets() to getPocket() to
match its return type Flow<Pocket> and the repo interface; update the function
declaration in PocketService (replace fun getPockets(): Flow<Pocket> with fun
getPocket(): Flow<Pocket>) and then update any callers or references to use
getPocket() so signatures remain consistent with the domain model and
repository.
In
`@feature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardScreen.kt`:
- Around line 75-97: snackbarHostState is created and used but never passed into
the UI, so MifosScaffold never shows snackbars; update PocketDashboardScreen to
forward snackbarHostState into PocketDashboardScreenContent (add parameter
passing of snackbarHostState) and then inside PocketDashboardScreenContent
supply it to MifosScaffold via snackbarHost = { SnackbarHost(snackbarHostState)
}; ensure the existing LaunchedEffect(state.snackbarMessage) remains to call
snackbarHostState.showSnackbar(...) and adjust function signatures
(PocketDashboardScreenContent) accordingly so the symbol names
snackbarHostState, PocketDashboardScreenContent, MifosScaffold, LaunchedEffect
and state.snackbarMessage are wired end-to-end.
In
`@feature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardViewModel.kt`:
- Around line 75-79: The snackbarMessage is left set on error in
PocketDashboardViewModel (mutableStateFlow update in the DataState.Error branch)
so repeated identical errors won't retrigger PocketDashboardScreen's
LaunchedEffect; add a clear action and handler: add
PocketDashboardAction.ClearSnackbar, handle it in handleAction to set
snackbarMessage = null on mutableStateFlow, and after showing the snackbar in
PocketDashboardScreen's LaunchedEffect call
viewModel.trySendAction(PocketDashboardAction.ClearSnackbar) so the message is
cleared and subsequent identical errors will retrigger the effect.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/DemoPocketRepository.kt`:
- Around line 84-85: The helper toPocketAccountType() silently maps unknown
strings to PocketAccountType.SAVINGS which can hide bugs; update to detect when
firstOrNull returns null and either (a) log a warning with the original String
and the fallback (include the function name toPocketAccountType and the constant
PocketAccountType.SAVINGS in the message) or (b) throw an explicit exception
(e.g., IllegalArgumentException) with the invalid value so failures are visible
during dev/testing—choose logging for leniency or throwing for strict failure,
and implement the chosen behavior inside toPocketAccountType().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9c8c911-99f4-4d01-8dbf-d6425cd9454d
📒 Files selected for processing (27)
cmp-shared/build.gradle.ktscmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/PocketRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/DemoPocketRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PocketRepositoryImpl.ktcore/model/src/commonMain/kotlin/org/mifospay/core/model/pocket/Pocket.ktcore/network/src/commonMain/kotlin/org/mifospay/core/network/KtorfitClient.ktcore/network/src/commonMain/kotlin/org/mifospay/core/network/SelfServiceApiManager.ktcore/network/src/commonMain/kotlin/org/mifospay/core/network/services/PocketService.ktcore/network/src/commonMain/kotlin/org/mifospay/core/network/services/ServiceExtensions.ktcore/network/src/commonMain/kotlin/org/mifospay/core/network/utils/ApiEndPoints.ktfeature/home/src/commonMain/composeResources/values/strings.xmlfeature/home/src/commonMain/kotlin/org/mifospay/feature/home/HomeScreen.ktfeature/home/src/commonMain/kotlin/org/mifospay/feature/home/HomeViewModel.ktfeature/home/src/commonMain/kotlin/org/mifospay/feature/home/navigation/HomeNavigation.ktfeature/pocket/build.gradle.ktsfeature/pocket/src/androidMain/AndroidManifest.xmlfeature/pocket/src/commonMain/composeResources/values/strings.xmlfeature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardScreen.ktfeature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardViewModel.ktfeature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/di/PocketModule.ktfeature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/link/LinkAccountScreen.ktfeature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/link/LinkAccountViewModel.ktfeature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/navigation/PocketNavigation.ktsettings.gradle.kts
Comment on lines
+578
to
+581
| linkAccountScreen( | ||
| navigateBack = navController::navigateUp, | ||
| onLinkSuccess = { navController.navigateToPocketDashboard() }, | ||
| ) |
Contributor
There was a problem hiding this comment.
Avoid pushing Pocket Dashboard twice after link success.
This callback navigates to dashboard even though success handling already navigates back from the link screen, which can leave duplicate dashboard entries in back stack.
💡 Suggested fix
linkAccountScreen(
navigateBack = navController::navigateUp,
- onLinkSuccess = { navController.navigateToPocketDashboard() },
+ onLinkSuccess = { /* no-op: LinkAccountScreen already navigates back on success */ },
)📝 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.
Suggested change
| linkAccountScreen( | |
| navigateBack = navController::navigateUp, | |
| onLinkSuccess = { navController.navigateToPocketDashboard() }, | |
| ) | |
| linkAccountScreen( | |
| navigateBack = navController::navigateUp, | |
| onLinkSuccess = { /* no-op: LinkAccountScreen already navigates back on success */ }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt`
around lines 578 - 581, The onLinkSuccess callback in the linkAccountScreen
currently calls navController.navigateToPocketDashboard() while navigateBack is
set to navController::navigateUp, causing duplicate Pocket Dashboard entries;
remove the explicit navController.navigateToPocketDashboard() call from the
onLinkSuccess lambda (or replace it with a no-op) so that success handling
relies solely on navigateBack/navController::navigateUp to return to the
dashboard and avoid pushing the dashboard twice.
| apiManager.pocketApi.getPockets() | ||
| .map<Pocket, DataState<Pocket>> { DataState.Success(it) } | ||
| .onStart { emit(DataState.Loading) } | ||
| .catch { emit(DataState.Error(it)) } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify broad exception catches and absence/presence of cancellation handling.
rg -nP --type=kotlin 'catch\s*\(\s*e:\s*Exception\s*\)' core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PocketRepositoryImpl.kt
rg -n --type=kotlin 'CancellationException' core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PocketRepositoryImpl.ktRepository: openMF/mifos-pay
Length of output: 132
🏁 Script executed:
cat -n core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PocketRepositoryImpl.ktRepository: openMF/mifos-pay
Length of output: 2625
Rethrow CancellationException to preserve cooperative cancellation.
Line 35 uses Flow.catch which catches all exceptions including CancellationException, converting cancellation into DataState.Error. This breaks cancellation semantics. The .catch block at line 35 must rethrow CancellationException.
Similarly, lines 43 and 53 should explicitly rethrow CancellationException for consistency and defense against any cancellation exceptions that may propagate.
Suggested fix
+import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.flow.Flow
@@
- .catch { emit(DataState.Error(it)) }
+ .catch {
+ if (it is CancellationException) throw it
+ emit(DataState.Error(it))
+ }
@@
- } catch (e: Exception) {
+ } catch (e: Exception) {
+ if (e is CancellationException) throw e
emit(DataState.Error(e))
}
@@
- } catch (e: Exception) {
+ } catch (e: Exception) {
+ if (e is CancellationException) throw e
emit(DataState.Error(e))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PocketRepositoryImpl.kt`
at line 35, The Flow.catch blocks in PocketRepositoryImpl that currently do
`.catch { emit(DataState.Error(it)) }` must preserve cooperative cancellation:
change each catch handler to rethrow CancellationException (e.g., if (it is
CancellationException) throw it) and only emit DataState.Error for
non-cancellation exceptions; update all three catch sites in
PocketRepositoryImpl (the catch wrapping Flow emissions that emit
DataState.Error) and ensure kotlin.coroutines.cancellation.CancellationException
is referenced/imported so cancellation is not swallowed.
Comment on lines
+23
to
+24
| @GET(ApiEndPoints.POCKETS) | ||
| fun getPockets(): Flow<Pocket> |
Contributor
There was a problem hiding this comment.
Fix method name to match return type.
The method name getPockets() is plural, but it returns Flow<Pocket> (singular). This creates a naming inconsistency. Based on the repository interface using getPocket() (singular) and the domain model structure (a single Pocket contains multiple linkedAccounts), the service method should be named getPocket() to match.
📝 Proposed fix
`@GET`(ApiEndPoints.POCKETS)
-fun getPockets(): Flow<Pocket>
+fun getPocket(): Flow<Pocket>📝 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.
Suggested change
| @GET(ApiEndPoints.POCKETS) | |
| fun getPockets(): Flow<Pocket> | |
| `@GET`(ApiEndPoints.POCKETS) | |
| fun getPocket(): Flow<Pocket> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/network/src/commonMain/kotlin/org/mifospay/core/network/services/PocketService.kt`
around lines 23 - 24, Rename the service method getPockets() to getPocket() to
match its return type Flow<Pocket> and the repo interface; update the function
declaration in PocketService (replace fun getPockets(): Flow<Pocket> with fun
getPocket(): Flow<Pocket>) and then update any callers or references to use
getPocket() so signatures remain consistent with the domain model and
repository.
Comment on lines
+75
to
+97
| val snackbarHostState = remember { SnackbarHostState() } | ||
|
|
||
| org.mifospay.core.ui.utils.EventsEffect(viewModel) { event -> | ||
| when (event) { | ||
| PocketDashboardEvent.NavigateBack -> navigateBack() | ||
| PocketDashboardEvent.NavigateToLinkAccount -> navigateToLinkAccount() | ||
| is PocketDashboardEvent.NavigateToAccountDetails -> navigateToAccountDetails(event.accountId) | ||
| } | ||
| } | ||
|
|
||
| LaunchedEffect(state.snackbarMessage) { | ||
| state.snackbarMessage?.let { snackbarHostState.showSnackbar(it) } | ||
| } | ||
|
|
||
| PocketDashboardScreenContent( | ||
| state = state, | ||
| onNavigateBack = { viewModel.trySendAction(PocketDashboardAction.NavigateBack) }, | ||
| onLinkAccount = { viewModel.trySendAction(PocketDashboardAction.NavigateToLinkAccount) }, | ||
| onDelinkAccount = { viewModel.trySendAction(PocketDashboardAction.DelinkAccount(it)) }, | ||
| onViewDetails = { viewModel.trySendAction(PocketDashboardAction.NavigateToAccountDetails(it)) }, | ||
| onRetry = { viewModel.trySendAction(PocketDashboardAction.Retry) }, | ||
| modifier = modifier, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check MifosScaffold signature for snackbarHost parameter
ast-grep --pattern $'@Composable
fun MifosScaffold($$$) {
$$$
}'Repository: openMF/mifos-pay
Length of output: 42
🏁 Script executed:
# Find MifosScaffold definition
fd -t f -i "*scaffold*" -e kt -e compose | head -20Repository: openMF/mifos-pay
Length of output: 350
🏁 Script executed:
# Search for MifosScaffold in the codebase
rg "fun MifosScaffold" -t kt -A 10Repository: openMF/mifos-pay
Length of output: 87
🏁 Script executed:
# Check the full PocketDashboardScreen file
cat -n feature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardScreen.kt | head -120Repository: openMF/mifos-pay
Length of output: 6417
🏁 Script executed:
# Search for MifosScaffold definition
rg "fun MifosScaffold" --type-list | headRepository: openMF/mifos-pay
Length of output: 450
🏁 Script executed:
# Try broader search for MifosScaffold
rg "MifosScaffold" -l | grep -i scaffoldRepository: openMF/mifos-pay
Length of output: 158
🏁 Script executed:
# Search in core designsystem
fd MifosScaffold -e ktRepository: openMF/mifos-pay
Length of output: 158
🏁 Script executed:
cat -n core/designsystem/src/commonMain/kotlin/org/mifospay/core/designsystem/component/MifosScaffold.kt | head -100Repository: openMF/mifos-pay
Length of output: 4873
🏁 Script executed:
sed -n '100,125p' feature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardScreen.ktRepository: openMF/mifos-pay
Length of output: 964
Wire the snackbar state to MifosScaffold to display error messages.
The snackbarHostState is created and messages are queued via LaunchedEffect, but the state is never passed to MifosScaffold. Since MifosScaffold accepts a snackbarHost parameter (defined in core/designsystem/src/commonMain/kotlin/org/mifospay/core/designsystem/component/MifosScaffold.kt), error messages—including delink failures—won't display to users without wiring this parameter.
Pass snackbarHostState from PocketDashboardScreen to PocketDashboardScreenContent, then add snackbarHost = { SnackbarHost(snackbarHostState) } to the MifosScaffold call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardScreen.kt`
around lines 75 - 97, snackbarHostState is created and used but never passed
into the UI, so MifosScaffold never shows snackbars; update
PocketDashboardScreen to forward snackbarHostState into
PocketDashboardScreenContent (add parameter passing of snackbarHostState) and
then inside PocketDashboardScreenContent supply it to MifosScaffold via
snackbarHost = { SnackbarHost(snackbarHostState) }; ensure the existing
LaunchedEffect(state.snackbarMessage) remains to call
snackbarHostState.showSnackbar(...) and adjust function signatures
(PocketDashboardScreenContent) accordingly so the symbol names
snackbarHostState, PocketDashboardScreenContent, MifosScaffold, LaunchedEffect
and state.snackbarMessage are wired end-to-end.
Comment on lines
+75
to
+79
| is DataState.Error -> mutableStateFlow.update { | ||
| it.copy( | ||
| isDelinking = false, | ||
| snackbarMessage = result.exception.message ?: "Failed to remove account", | ||
| ) |
Contributor
There was a problem hiding this comment.
Clear snackbar message after displaying to allow repeated errors.
The snackbarMessage is set on error but never cleared. Because the LaunchedEffect in PocketDashboardScreen (line 85) is keyed on state.snackbarMessage, if two consecutive delink operations fail with the same error text, the second error won't trigger a new snackbar because the key hasn't changed.
🔧 Proposed fix to clear the message after setting
Option 1: Add a timestamp to make each error unique:
is DataState.Error -> mutableStateFlow.update {
it.copy(
isDelinking = false,
- snackbarMessage = result.exception.message ?: "Failed to remove account",
+ snackbarMessage = "${result.exception.message ?: "Failed to remove account"} [${System.currentTimeMillis()}]",
)
}Option 2: Add a separate action to clear the message (cleaner approach):
Add to PocketDashboardAction:
data object ClearSnackbar : PocketDashboardActionHandle in handleAction:
is PocketDashboardAction.ClearSnackbar -> mutableStateFlow.update { it.copy(snackbarMessage = null) }Then in the Screen, after showing the snackbar:
LaunchedEffect(state.snackbarMessage) {
state.snackbarMessage?.let {
snackbarHostState.showSnackbar(it)
viewModel.trySendAction(PocketDashboardAction.ClearSnackbar)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/pocket/src/commonMain/kotlin/org/mifospay/feature/pocket/dashboard/PocketDashboardViewModel.kt`
around lines 75 - 79, The snackbarMessage is left set on error in
PocketDashboardViewModel (mutableStateFlow update in the DataState.Error branch)
so repeated identical errors won't retrigger PocketDashboardScreen's
LaunchedEffect; add a clear action and handler: add
PocketDashboardAction.ClearSnackbar, handle it in handleAction to set
snackbarMessage = null on mutableStateFlow, and after showing the snackbar in
PocketDashboardScreen's LaunchedEffect call
viewModel.trySendAction(PocketDashboardAction.ClearSnackbar) so the message is
cleared and subsequent identical errors will retrigger the effect.
Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
bhuvan-somisetty
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt (1)
577-580:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid pushing Pocket Dashboard twice on link success.
navigateBackalready handles returning from the link screen; the extra navigate at Line 579 can duplicate dashboard in the back stack.💡 Minimal fix
linkAccountScreen( navigateBack = navController::navigateUp, - onLinkSuccess = { navController.navigateToPocketDashboard() }, + onLinkSuccess = { navController.navigateUp() }, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt` around lines 577 - 580, The linkAccountScreen call is causing the Pocket Dashboard to be pushed twice because navigateBack (navController::navigateUp) already returns from the link screen; remove the extra navigation in the onLinkSuccess callback (the navController.navigateToPocketDashboard() invocation) or change onLinkSuccess to a no-op so only navigateBack is used; update the linkAccountScreen invocation (referencing linkAccountScreen, navigateBack, onLinkSuccess, and navController.navigateToPocketDashboard) accordingly to avoid duplicating the dashboard in the back stack.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt`:
- Around line 577-580: The linkAccountScreen call is causing the Pocket
Dashboard to be pushed twice because navigateBack (navController::navigateUp)
already returns from the link screen; remove the extra navigation in the
onLinkSuccess callback (the navController.navigateToPocketDashboard()
invocation) or change onLinkSuccess to a no-op so only navigateBack is used;
update the linkAccountScreen invocation (referencing linkAccountScreen,
navigateBack, onLinkSuccess, and navController.navigateToPocketDashboard)
accordingly to avoid duplicating the dashboard in the back stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72c5546b-263f-4a18-ae40-9f2e73488817
📒 Files selected for processing (3)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.ktcore/network/src/commonMain/kotlin/org/mifospay/core/network/KtorfitClient.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- core/network/src/commonMain/kotlin/org/mifospay/core/network/KtorfitClient.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proof of concept for the Pocket feature (issue #1997, MW-378). Covers three of the four defined goals: viewing the pocket dashboard, linking accounts, and delinking accounts.
The pocket dashboard shows all linked savings, loan, and share accounts in one view with an aggregated total balance card. A shortcut card on the home screen navigates to it. Linking an account takes an account ID and type; delinking shows a confirmation dialog before removing the account from the list.
The data layer uses an in-memory demo repository so the feature is fully interactive without a running backend. The real
PocketRepositoryImplwired to the Fineract self-service pocket API (GET /self/pockets,POST /self/pockets?command=linkAccounts,POST /self/pockets?command=delinkAccounts) is also included and can be swapped into the DI module once the API is reachable.Navigation to the individual account detail screen is wired but the destination itself is out of scope for this POC.
Tested manually: dashboard with pre-loaded accounts (savings + loan), link/delink flow, empty state after removing all accounts, and the home screen shortcut.
Closes #1997
Summary by CodeRabbit