Feature/pocket repository conflict resolution - #3152
Open
Kartikey15dem wants to merge 11 commits into
Open
Conversation
feat(core/network):implement pocket repository feat(core/network):implement pocket repository feat(core/network):implement pocket repository # Conflicts: # core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/di/RepositoryModule.kt # core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/mapper/pocket/PocketAccountMapper.kt # core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repository/PocketRepository.kt # core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/entity/pocket/PocketAccount.kt
feat(feat/pocket):implement pocket dashboard screen feat(feature/pocket): implement pocket dashboard screen feat(feature/pocket): implement pocket dashboard screen # Conflicts: # cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt # feature/home/src/commonMain/composeResources/values/strings.xml # feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardScreen.kt # feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt # Conflicts: # cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt # feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardScreen.kt # feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt
feat(feature/pocket): add pull to refresh functionality
feat(core/data): rebased onto pocket repository changes
fix(cmp-navigation): implement platform specific database module
fix(core/data): fix web application checks failing
feat(core/data): add conflict resolution in pocket repository
Contributor
📝 WalkthroughWalkthroughThe change adds pocket cache reset and local/server reconciliation. The dashboard clears cached data before loading, calculates totals by currency, and updates pocket presentation and native currency formatting. ChangesPocket cache and dashboard updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PocketDashboardViewModel
participant PocketRepositoryImp
participant PocketServer
participant PocketAccountDao
PocketDashboardViewModel->>PocketRepositoryImp: resetPocketCache()
PocketDashboardViewModel->>PocketRepositoryImp: loadPocketData()
PocketRepositoryImp->>PocketServer: reconcile pocket mappings
PocketRepositoryImp->>PocketAccountDao: replaceAllPocketAccounts()
PocketRepositoryImp-->>PocketDashboardViewModel: return local pocket accounts
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt (1)
43-46: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResetting the cache on every init defeats the repository cache.
resetPocketCache()clearsdetailedPocketCacheandcachedClientIdeach time the ViewModel is created.loadPocketData()then callsgetDetailedPocketAccounts(clientId, forceRefresh = false), which now always misses the cache. Every navigation to the dashboard triggerssyncPocketsWithServer(), a client-accounts fetch, and one request per share account.If the goal is fresh data on entry, call
loadPocketData(forceRefresh = true)instead. That keeps the cache useful for other consumers such asgetAvailableAccountsToLink, which readsdetailedPocketCachedirectly.♻️ Proposed change
init { - viewModelScope.launch { - pocketRepository.resetPocketCache() - loadPocketData() - } + loadPocketData(forceRefresh = true) }🤖 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/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt` around lines 43 - 46, Remove the pocketRepository.resetPocketCache() call from the ViewModel initialization flow and invoke loadPocketData with forceRefresh = true instead. Update loadPocketData and its call to getDetailedPocketAccounts as needed to propagate the flag, preserving detailedPocketCache for other consumers such as getAvailableAccountsToLink.core/data/src/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt (1)
314-320: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
syncPocketsin the else branch runs a full network reload inside a delink.When
detailedPocketCacheholds no success value, line 319 triggerssyncPockets(clientId, forceRefresh = true). That call performssyncPocketsWithServer()plus a client-accounts fetch and per-share-account requests, before the server delink at line 326 has run. The reconciliation therefore observes the pocket as server-only and deletes it as a side effect, which duplicates the explicit delete below.Move the server delink before the cache refresh, so the refresh observes the final state.
🤖 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/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt` around lines 314 - 320, The delink flow refreshes the pocket cache before applying the server-side delink, causing syncPockets to observe stale server state and perform duplicate deletion. In the surrounding delink method, move the server delink operation before the detailedPocketCache handling and its syncPockets(clientId, forceRefresh = true) fallback, while preserving the existing cache filtering behavior afterward.core/database/src/commonMain/kotlin/org/mifos/mobile/core/database/dao/PocketAccountDao.kt (1)
28-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider adding a transactional replace helper.
deleteAll()is called immediately beforelinkPocketAccounts()inPocketRepositoryImp(nonJsCommonMain), insyncPocketsWithServer()and inlinkAccounts(). Without a transaction, a failure between the two calls leaves thepocketstable empty. A@Transactionmethod in the DAO makes the replace atomic for every caller.♻️ Proposed transactional replace
`@Query`("DELETE FROM pockets") suspend fun deleteAll() + + `@Transaction` + suspend fun replaceAllPocketAccounts(pockets: List<PocketAccountEntity>) { + deleteAll() + linkPocketAccounts(pockets) + } }Add
import androidx.room.Transaction.🤖 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/database/src/commonMain/kotlin/org/mifos/mobile/core/database/dao/PocketAccountDao.kt` around lines 28 - 30, Update PocketAccountDao by adding a `@Transaction` replace helper that performs deleteAll() followed by linkPocketAccounts() atomically, and have PocketRepositoryImp callers such as syncPocketsWithServer() and linkAccounts() use this helper instead of invoking the two DAO methods separately. Add the required androidx.room.Transaction import while preserving existing linking behavior.
🤖 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
`@core/data/src/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt`:
- Around line 273-306: Update the linkAccounts flow in PocketRepositoryImp so
server-side rejection errors from dataManager.pocketApi.linkAccounts are
propagated to the caller instead of being swallowed by the catch block. Preserve
the existing successful synchronization and offline/transport-failure behavior,
allowing retries for transient failures while distinguishing and surfacing
validation or 4xx-style server rejections before reporting success.
- Around line 61-78: Update syncPocketsWithServer() so accountsToDelink is not
derived from server pockets absent in localBasicPockets; until explicit local
deletion intent exists, skip server delinking entirely. Preserve server pockets
missing locally by merging/inserting them into the local database, and only
reintroduce deletion after using a pending-delete flag or equivalent outbox
state.
- Around line 240-249: Update the temporary ID generation in linkPocketAccounts
so separate link operations cannot produce overlapping negative ID ranges;
define and use a companion-object ID_BLOCK_SIZE larger than the maximum batch
size when combining the epoch-millisecond value with the item index, while
preserving unique per-account IDs within each operation.
- Around line 72-76: Update every silent catch block in PocketRepositoryImp,
including those in the delink and sync operations, to rethrow
CancellationException before handling other failures. Log non-cancellation
exceptions with the repository’s existing logging mechanism instead of ignoring
them, while preserving the current recovery behavior after logging.
In
`@core/database/src/commonMain/kotlin/org/mifos/mobile/core/database/dao/PocketAccountDao.kt`:
- Around line 28-30: The pocket table replacement is non-atomic, allowing
deletion to persist if reinsertion fails or the process stops. In
core/database/src/commonMain/kotlin/org/mifos/mobile/core/database/dao/PocketAccountDao.kt:28-30,
import androidx.room.Transaction and add transactional
replaceAllPocketAccounts(pockets), calling deleteAll() followed by
linkPocketAccounts(pockets). In
core/data/src/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt:116-117,
update syncPocketsWithServer() to call replaceAllPocketAccounts with the mapped
localBasicPockets; at :257-258, update linkAccounts() to call it with the mapped
allLocalPockets.
In
`@feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt`:
- Around line 175-184: Update the fallback in PocketDashboardViewModel’s
formattedTotal calculation to retain the previous totalBalance value when
CurrencyFormatter.format returns an empty string, particularly when no account
has a currency code. Preserve the zero-balance formatting for valid currency
data and ensure the resulting state never replaces a usable default or previous
value with an empty string.
---
Nitpick comments:
In
`@core/data/src/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt`:
- Around line 314-320: The delink flow refreshes the pocket cache before
applying the server-side delink, causing syncPockets to observe stale server
state and perform duplicate deletion. In the surrounding delink method, move the
server delink operation before the detailedPocketCache handling and its
syncPockets(clientId, forceRefresh = true) fallback, while preserving the
existing cache filtering behavior afterward.
In
`@core/database/src/commonMain/kotlin/org/mifos/mobile/core/database/dao/PocketAccountDao.kt`:
- Around line 28-30: Update PocketAccountDao by adding a `@Transaction` replace
helper that performs deleteAll() followed by linkPocketAccounts() atomically,
and have PocketRepositoryImp callers such as syncPocketsWithServer() and
linkAccounts() use this helper instead of invoking the two DAO methods
separately. Add the required androidx.room.Transaction import while preserving
existing linking behavior.
In
`@feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt`:
- Around line 43-46: Remove the pocketRepository.resetPocketCache() call from
the ViewModel initialization flow and invoke loadPocketData with forceRefresh =
true instead. Update loadPocketData and its call to getDetailedPocketAccounts as
needed to propagate the flag, preserving detailedPocketCache for other consumers
such as getAvailableAccountsToLink.
🪄 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 Plus
Run ID: 38c7ceb4-b059-4d00-a4e0-380c49b71151
📒 Files selected for processing (9)
core/common/src/nativeMain/kotlin/org/mifos/mobile/core/common/CurrencyFormatter.native.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repository/PocketRepository.ktcore/data/src/jsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.ktcore/data/src/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.ktcore/database/src/commonMain/kotlin/org/mifos/mobile/core/database/dao/PocketAccountDao.ktcore/database/src/desktopMain/kotlin/org/mifos/mobile/core/database/di/DatabaseModule.desktop.ktcore/database/src/nativeMain/kotlin/org/mifos/mobile/core/database/di/DatabaseModule.native.ktfeature/pocket/src/commonMain/composeResources/values/strings.xmlfeature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt
There was a problem hiding this comment.
Pull request overview
This PR updates the Pocket feature’s data layer to better handle local/server pocket-linking conflicts by syncing local pocket mappings to the server, resetting pocket caches on dashboard load, and improving multi-currency balance presentation.
Changes:
- Added a local↔server reconciliation step for pocket mappings (link/delink) and updated repository caching behavior.
- Updated pocket dashboard total balance logic to display totals per currency.
- Adjusted Room database wiring (typed dispatcher injection) and added a DAO helper for clearing pocket mappings.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| feature/pocket/src/commonMain/kotlin/org/mifos/mobile/feature/pocket/pocketDashboard/PocketDashboardViewModel.kt | Resets pocket cache on init; changes total-balance formatting to multi-currency totals. |
| feature/pocket/src/commonMain/composeResources/values/strings.xml | Updates search hint copy. |
| core/database/src/nativeMain/kotlin/org/mifos/mobile/core/database/di/DatabaseModule.native.kt | Uses typed CoroutineDispatcher resolution for Room query context. |
| core/database/src/desktopMain/kotlin/org/mifos/mobile/core/database/di/DatabaseModule.desktop.kt | Uses typed CoroutineDispatcher resolution for Room query context. |
| core/database/src/commonMain/kotlin/org/mifos/mobile/core/database/dao/PocketAccountDao.kt | Adds deleteAll() for pocket mappings (used by new sync flow). |
| core/data/src/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt | Implements reconciliation logic, local-first updates for link/delink, and adds resetPocketCache(). |
| core/data/src/jsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt | Adds no-op resetPocketCache() for JS target. |
| core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repository/PocketRepository.kt | Adds resetPocketCache() to repository interface. |
| core/common/src/nativeMain/kotlin/org/mifos/mobile/core/common/CurrencyFormatter.native.kt | Updates native currency formatter configuration to set currencyCode. |
Suppressed comments (1)
core/data/src/nonJsCommonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/PocketRepositoryImp.kt:258
- Same non-atomic pattern here:
deleteAll()followed bylinkPocketAccounts()can leave the table empty if the insert throws. Use the transactional DAO helper (e.g.,replaceAll(...)) to make the update atomic.
pocketAccountDao.deleteAll()
pocketAccountDao.linkPocketAccounts(allLocalPockets.map { it.toEntity() })
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Fixes - Jira-#590
Implemented conflict resolution for the pocket repository to avoid ambiguity between pocket network APIs and offline fallback.
Summary by CodeRabbit
New Features
Bug Fixes