Refactor: Improve state management and code organization#213
Refactor: Improve state management and code organization#213Mohammad-Faiz-Cloud-Engineer wants to merge 10 commits intorainxchzed:mainfrom
Conversation
WalkthroughThis PR restructures startup and observation flows in MainViewModel, adds a MigrateInstalledAppsDataUseCase and DI wiring for it, consolidates theme/status observers into combined flows, and refactors developer-profile and home features for paginated fetching, concurrent release checks, favorites handling, and inline repository status application. Changes
Sequence Diagram(s)sequenceDiagram
participant MainVM as MainViewModel
participant DI as DI Container
participant Migrator as MigrateInstalledAppsDataUseCase
participant Repo as InstalledAppsRepository
participant PM as PackageMonitor
participant PF as Platform
MainVM->>DI: request MainViewModel (startup)
DI->>MainVM: inject MigrateInstalledAppsDataUseCase, InstalledAppsRepository
MainVM->>Migrator: invoke() (performInitialMigration)
Migrator->>PM: getInstalledSystemPackages()
Migrator->>Repo: getAllInstalledApps()
Migrator->>Migrator: for each app: decide delete or migrate
Migrator->>PF: (platform check) get package info if ANDROID
Migrator->>Repo: update or delete app records
Migrator-->>MainVM: Result(success/failure)
MainVM->>Repo: installedAppsRepository.checkAllForUpdates() (post-migration)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/home/presentation/HomeViewModel.kt (1)
160-174:⚠️ Potential issue | 🟠 MajorComment claims parallel execution but code is sequential.
The comment says "Fetch all status data in parallel" but the three
.first()calls execute sequentially—each must complete before the next starts. Either fix the comment to reflect sequential execution, or useasyncfor true parallelism.🔧 Option 1: Fix comment to reflect actual behavior
- // Fetch all status data in parallel + // Fetch all status data sequentially⚡ Option 2: Implement true parallel fetching
- // Fetch all status data in parallel - val installedAppsMap = installedAppsRepository - .getAllInstalledApps() - .first() - .associateBy { it.repoId } - - val favoritesMap = favouritesRepository - .getAllFavorites() - .first() - .associateBy { it.repoId } - - val starredReposMap = starredRepository - .getAllStarred() - .first() - .associateBy { it.repoId } + // Fetch all status data in parallel + val installedAppsDeferred = async { + installedAppsRepository.getAllInstalledApps().first().associateBy { it.repoId } + } + val favoritesDeferred = async { + favouritesRepository.getAllFavorites().first().associateBy { it.repoId } + } + val starredDeferred = async { + starredRepository.getAllStarred().first().associateBy { it.repoId } + } + + val installedAppsMap = installedAppsDeferred.await() + val favoritesMap = favoritesDeferred.await() + val starredReposMap = starredDeferred.await()
🤖 Fix all issues with AI agents
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/developer_profile/presentation/DeveloperProfileViewModel.kt`:
- Around line 201-236: toggleFavorite currently updates _state and calls
applyFiltersAndSort regardless of whether favouritesRepository.toggleFavorite
succeeded, causing UI/db inconsistency; wrap the call to
favouritesRepository.toggleFavorite(repo) in a try/catch (or check its success
return) inside toggleFavorite, and only update _state (the block that maps
repositories and calls applyFiltersAndSort) when the repository operation
succeeds, otherwise log/handle the error and do not flip isFavorite (or revert
the change if you optimistically updated first). Refer to the toggleFavorite
function, favouritesRepository.toggleFavorite, the _state.update mapping that
flips isFavorite, and applyFiltersAndSort to locate where to gate the UI update.
🧹 Nitpick comments (4)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/home/presentation/HomeViewModel.kt (1)
70-77: Consider adding debug-level logging for silent failures.While suppressing non-critical failures is reasonable, completely silent exception swallowing can make troubleshooting difficult. A debug-level log would help identify issues without impacting production.
💡 Optional: Add debug logging
try { syncInstalledAppsUseCase() } catch (e: Exception) { - // Silent fail - non-critical operation + Logger.d { "Non-critical sync failed: ${e.message}" } }composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/domain/use_cases/MigrateInstalledAppsDataUseCase.kt (2)
24-48: Consider convertinginstalledPackageNamesto a Set for O(1) lookups.The
!in installedPackageNamescheck on line 32 is O(n) ifgetAllInstalledPackageNames()returns a List. With many apps, this results in O(n²) complexity. The PR description mentions using HashSet for favorite ID lookups inDeveloperProfileRepositoryImpl— the same optimization would benefit this migration logic.♻️ Proposed fix
suspend operator fun invoke(): Result<Unit> { return try { - val installedPackageNames = packageMonitor.getAllInstalledPackageNames() + val installedPackageNames = packageMonitor.getAllInstalledPackageNames().toSet() val appsInDb = installedAppsRepository.getAllInstalledApps().first()
55-92: Minor duplication between Android fallback and desktop case.Lines 72-79 and 83-90 are identical. Consider extracting to reduce duplication, though this is a low-priority refactor.
♻️ Optional: Extract common fallback logic
private suspend fun migrateAppVersionData(packageName: String) { val app = installedAppsRepository.getAppByPackage(packageName) ?: return if (platform.type == PlatformType.ANDROID) { val systemInfo = packageMonitor.getInstalledPackageInfo(packageName) if (systemInfo != null) { // Use system package info installedAppsRepository.updateApp( app.copy( installedVersionName = systemInfo.versionName, installedVersionCode = systemInfo.versionCode, latestVersionName = systemInfo.versionName, latestVersionCode = systemInfo.versionCode ) ) - } else { - // Fallback to legacy tag - installedAppsRepository.updateApp( - app.copy( - installedVersionName = app.installedVersion, - installedVersionCode = 0L, - latestVersionName = app.installedVersion, - latestVersionCode = 0L - ) - ) + return } - } else { - // Desktop platforms: use legacy tag as version name - installedAppsRepository.updateApp( - app.copy( - installedVersionName = app.installedVersion, - installedVersionCode = 0L, - latestVersionName = app.installedVersion, - latestVersionCode = 0L - ) + } + + // Fallback for desktop or when Android system info unavailable + installedAppsRepository.updateApp( + app.copy( + installedVersionName = app.installedVersion, + installedVersionCode = 0L, + latestVersionName = app.installedVersion, + latestVersionCode = 0L ) - } + ) }composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/SharedModules.kt (1)
109-115: Consider using simple class name with import for consistency.
MigrateInstalledAppsDataUseCaseuses the fully qualified name, whileSyncInstalledAppsUseCase(line 101) uses the simple name. Adding an import would maintain consistency across the module.♻️ Proposed fix
Add to imports:
import zed.rainxch.githubstore.core.domain.use_cases.MigrateInstalledAppsDataUseCaseThen update the singleton:
- single<zed.rainxch.githubstore.core.domain.use_cases.MigrateInstalledAppsDataUseCase> { - zed.rainxch.githubstore.core.domain.use_cases.MigrateInstalledAppsDataUseCase( + single<MigrateInstalledAppsDataUseCase> { + MigrateInstalledAppsDataUseCase( packageMonitor = get(), installedAppsRepository = get(), platform = get() ) }
.../zed/rainxch/githubstore/feature/developer_profile/presentation/DeveloperProfileViewModel.kt
Show resolved
Hide resolved
|
Hey @Mohammad-Faiz-Cloud-Engineer, thank you so much for this huge and thoughtful refactor — seriously, I really appreciate the time and effort you put into improving the architecture and cleanup here 🙌 Right now I’m in the middle of a large migration from a single-module setup to a multi-module architecture: Because of that, the project structure and many of the same files you touched are actively changing. If I try to merge this PR now, we’ll likely end up with a lot of heavy merge conflicts and risk breaking things during the migration. So the best approach would be:
Your changes and ideas here are still very valuable, and I’d definitely like to bring them in once the migration dust settles. If you’re up for it later, you could rebase on the updated structure — otherwise I can also help port the improvements over. Thanks again for the great contribution and for your patience! 🚀 |
|
Hey @rainxchzed, thanks for the heads up and totally understand! 👍 Makes complete sense to wait for the multi-module migration to finish first - definitely don't want to create merge conflicts or break anything during that process. I'll close this PR for now and keep the branch around. Once #210 is merged and the new structure is in place, I'm happy to rebase these changes on top of it. The refactoring should still be relevant since it's mostly about cleaning up the ViewModels and state management logic. Just ping me when you're ready and I can either: Rebase and update the PR myself |
|
@Mohammad-Faiz-Cloud-Engineer Yeah i definitely do, thanks for understanding the situation! |
2 participants
This PR refactors several ViewModels and repositories to improve code maintainability and performance. The main focus was on cleaning up the developer profile feature and optimizing how we observe data changes throughout the app.
Changes
ViewModels
Repositories
substringBeforeinstead ofsplitNew Files
Other
Why?
The main motivation was to clean up some technical debt, especially around the developer profile feature. The code was working fine but had some areas that could be more maintainable:
Testing
Notes
This is purely a refactoring PR - no new features or bug fixes. All existing functionality should work exactly the same as before.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.