fix: restore @query annotations on InstalledAppDao monorepo lookups#483
Merged
rainxchzed merged 4 commits intomainfrom May 2, 2026
Merged
fix: restore @query annotations on InstalledAppDao monorepo lookups#483rainxchzed merged 4 commits intomainfrom
rainxchzed merged 4 commits intomainfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR converts home discovery from single-platform selection to multi-platform selection across domain, data, repository, and presentation layers; adds a DAO method to list installed apps by repo; and makes PackageEventReceiver's broadcast handling null-safe. ChangesMulti-Platform Discovery Selection
Android Package Receiver & App DAO Support
Sequence DiagramsequenceDiagram
participant User
participant UI as HomeRoot (UI)
participant VM as HomeViewModel
participant Repo as HomeRepository
participant DB as Local Cache
User->>UI: Toggle platform in popup
UI->>VM: dispatch(TogglePlatform(platform))
VM->>VM: Update selectedPlatforms set
VM->>VM: Call loadRepos(platforms=targetPlatforms)
VM->>Repo: getTrendingRepositories(platforms=targetPlatforms)
Repo->>Repo: Normalize platforms → topics filter
Repo->>DB: Check local cache (cacheKey: category, platforms, page)
alt Cache miss
Repo->>Repo: Fetch from GitHub with buildSimplifiedQuery(platforms)
Repo->>DB: Write paginated results
end
DB-->>Repo: Cached/fresh repos
Repo-->>VM: PaginatedDiscoveryRepositories
VM->>VM: Update state.repositories
VM-->>UI: Emit new HomeState
UI->>UI: Re-render with selected platforms + filtered repos
UI-->>User: Display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🧹 Nitpick comments (5)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt (1)
107-111: ⚡ Quick win
scope.launchshould begetBackstopScope().launchto survive receiver teardown on the manifest-registered path.The local
scope(CoroutineScope(SupervisorJob() + Dispatchers.IO), line 52) is tied to the receiver instance. Android may destroy the instance immediately afteronReceivereturns, cutting off any work launched here.getBackstopScope()exists precisely to hand off to the Koin app scope in that case — yet the two dispatch sites usescopedirectly, while every subsequent async operation (onPackageInstalledline 186,handleExternalInstallline 302, retry line 328) correctly usesgetBackstopScope(). This leavesonPackageInstalledandonPackageRemovedsilently cancellable at the point of entry.♻️ Proposed fix
Intent.ACTION_PACKAGE_ADDED, Intent.ACTION_PACKAGE_REPLACED, Intent.ACTION_MY_PACKAGE_REPLACED, -> { - scope.launch { onPackageInstalled(packageName) } + getBackstopScope().launch { onPackageInstalled(packageName) } } Intent.ACTION_PACKAGE_FULLY_REMOVED -> { - scope.launch { onPackageRemoved(packageName) } + getBackstopScope().launch { onPackageRemoved(packageName) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt` around lines 107 - 111, The receiver uses a local CoroutineScope named scope to launch work that can be cancelled when the BroadcastReceiver instance is torn down; replace those launches with the app-level scope by calling getBackstopScope().launch instead of scope.launch for the package events so work survives receiver teardown — specifically change the two dispatch sites that call scope.launch (the ACTION_PACKAGE_ADDED branch that launches onPackageInstalled(packageName) and the ACTION_PACKAGE_FULLY_REMOVED branch that launches onPackageRemoved(packageName)) to use getBackstopScope().launch so they match other async paths (onPackageInstalled, handleExternalInstall, retry logic) that already use getBackstopScope().feature/home/data/src/commonMain/kotlin/zed/rainxch/home/data/repository/HomeRepositoryImpl.kt (1)
562-566: 💤 Low value
normalize()contains a redundantAll-filter on line 564Line 563 returns early whenever
Allis in the set, so by the time execution reaches line 564,Allis guaranteed not to be inthis. Thefilter { it != DiscoveryPlatform.All }is therefore always a no-op:🔧 Proposed cleanup
private fun Set<DiscoveryPlatform>.normalize(): Set<DiscoveryPlatform> { if (contains(DiscoveryPlatform.All)) return emptySet() - val real = filter { it != DiscoveryPlatform.All }.toSet() + val real = this // `All` already handled above return if (real.size == DiscoveryPlatform.selectablePlatforms.size) emptySet() else real }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/data/src/commonMain/kotlin/zed/rainxch/home/data/repository/HomeRepositoryImpl.kt` around lines 562 - 566, The normalize() extension on Set<DiscoveryPlatform> redundantly filters out DiscoveryPlatform.All after already returning early when All is present; remove the unnecessary filter and simply use the current set (e.g., val real = this.toSet() or val real = this) when computing real, then keep the existing size comparison against DiscoveryPlatform.selectablePlatforms to decide whether to return emptySet() or real; update the body of normalize() accordingly to eliminate the no-op filter of DiscoveryPlatform.All.feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeViewModel.kt (2)
320-340: 💤 Low valueSequential live search per topic may skip remaining topics on failure.
If
homeRepository.searchByTopic(...).collectthrows for one topic, the exception propagates to the outer catch block, skipping subsequent topics in theforEach. Consider wrapping individual topic searches to isolate failures:♻️ Isolate per-topic failures
topics.forEach { topic -> + try { homeRepository .searchByTopic( searchKeywords = topic.searchKeywords, platforms = platforms, page = 1, ).collect { paginatedRepos -> val newReposWithStatus = mapReposToUi(paginatedRepos.repos) _state.update { currentState -> val merged = (currentState.repos + newReposWithStatus) .distinctBy { it.repository.fullName } currentState.copy( repos = merged.toImmutableList(), hasMorePages = currentState.hasMorePages || paginatedRepos.hasMore, ) } } + } catch (t: Throwable) { + if (t is CancellationException) throw t + logger.warn("Topic supplement search for $topic failed: ${t.message}") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeViewModel.kt` around lines 320 - 340, The forEach over topics in HomeViewModel.kt currently calls homeRepository.searchByTopic(...).collect which, if it throws for one topic, aborts the loop and skips remaining topics; wrap each per-topic search/collect in its own try-catch (or use runCatching) around the call to homeRepository.searchByTopic and the mapping via mapReposToUi so that any exception is caught, logged (or handled) and the loop continues, while preserving the existing _state.update behavior for successful results.
162-172: 💤 Low valueAsync deferred evaluated unconditionally when platforms is null.
The
targetPlatformsDeferredis always launched but only awaited whenplatforms == null. Whenplatformsis provided, the async job still runs to completion in the background unnecessarily.Consider making the deferred lazy or only launching when needed:
♻️ Suggested improvement
- val targetPlatformsDeferred = - viewModelScope.async { - tweaksRepository.getDiscoveryPlatforms().first() - } val targetTopics = if (topicsExplicitlySet) topics.orEmpty() else _state.value.selectedTopics logger.debug("Loading repos: category=$targetCategory, topics=$targetTopics, page=$nextPageIndex, isInitial=$isInitial") return viewModelScope .launch { - val targetPlatforms = platforms ?: targetPlatformsDeferred.await() + val targetPlatforms = platforms ?: tweaksRepository.getDiscoveryPlatforms().first()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeViewModel.kt` around lines 162 - 172, In HomeViewModel, avoid launching the background async job unconditionally: the targetPlatformsDeferred created via viewModelScope.async is started even when platforms is supplied; either create the deferred with viewModelScope.async(start = CoroutineStart.LAZY) and only call await() when platforms == null, or move the viewModelScope.async call inside the branch where platforms is null so it only starts when needed; update references to targetPlatformsDeferred.await() used in the launch block (and keep logger/variable names: targetPlatformsDeferred, platforms, targetPlatforms) so the deferred is only started/awaited when required.feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.kt (1)
744-758: 💤 Low valueSelection indicator causes layout shift.
When
isSelectedtoggles, the Done icon conditionally renders, causing the text to shift horizontally. Consider using a fixed-width container orModifier.alpha(0f)for the unselected state to maintain stable layout:♻️ Stable layout with invisible placeholder
Row( verticalAlignment = Alignment.CenterVertically, horizontalArrangement = Arrangement.spacedBy( 6.dp, Alignment.Start, ), ) { - if (isSelected) { - Icon( - imageVector = Icons.Default.Done, - contentDescription = null, - tint = MaterialTheme.colorScheme.primary, - modifier = Modifier.size(20.dp), - ) - } + Icon( + imageVector = Icons.Default.Done, + contentDescription = null, + tint = if (isSelected) MaterialTheme.colorScheme.primary else Color.Transparent, + modifier = Modifier.size(20.dp), + ) Text( text = label,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.kt` around lines 744 - 758, The conditional Icon rendering makes the Text shift when isSelected toggles; always reserve the icon space instead: render the Icon unconditionally (the same Icon call in HomeRoot.kt) but set its visibility via Modifier.alpha(if (isSelected) 1f else 0f) or wrap it in a fixed-width container/Spacer matching Modifier.size(20.dp) so the Text (the Text(...) call) keeps a stable position when isSelected changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.kt`:
- Around line 152-165: The legacy-branch in getDiscoveryPlatforms() reads
DISCOVERY_PLATFORM_KEY but never writes DISCOVERY_PLATFORMS_KEY, so migration
never completes; modify the branch so that when stored == null and a legacy
non-All value is found, perform a one-shot write-through to backfill
DISCOVERY_PLATFORMS_KEY (using preferences.edit to set the new key to a set
containing the migrated platform) and then return that set; ensure the write
only happens when migrating (stored == null && legacy != All) to avoid loops and
leave the rest of the mapping behavior unchanged (references:
getDiscoveryPlatforms(), DISCOVERY_PLATFORMS_KEY, DISCOVERY_PLATFORM_KEY,
setDiscoveryPlatforms).
In
`@feature/home/data/src/commonMain/kotlin/zed/rainxch/home/data/repository/HomeRepositoryImpl.kt`:
- Around line 545-549: The current query builder in HomeRepositoryImpl (the when
block that inspects topics and uses baseQuery and topics.joinToString)
incorrectly groups multiple topic qualifiers with parentheses, which GitHub REST
search treats as literal text; change the multi-topic branch to repeat the full
baseQuery for each topic and join those full queries with " OR " (e.g.,
topics.joinToString(" OR ") { "$baseQuery topic:$it" }) so each topic becomes
its own complete query clause instead of a parenthesized group.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt`:
- Around line 107-111: The receiver uses a local CoroutineScope named scope to
launch work that can be cancelled when the BroadcastReceiver instance is torn
down; replace those launches with the app-level scope by calling
getBackstopScope().launch instead of scope.launch for the package events so work
survives receiver teardown — specifically change the two dispatch sites that
call scope.launch (the ACTION_PACKAGE_ADDED branch that launches
onPackageInstalled(packageName) and the ACTION_PACKAGE_FULLY_REMOVED branch that
launches onPackageRemoved(packageName)) to use getBackstopScope().launch so they
match other async paths (onPackageInstalled, handleExternalInstall, retry logic)
that already use getBackstopScope().
In
`@feature/home/data/src/commonMain/kotlin/zed/rainxch/home/data/repository/HomeRepositoryImpl.kt`:
- Around line 562-566: The normalize() extension on Set<DiscoveryPlatform>
redundantly filters out DiscoveryPlatform.All after already returning early when
All is present; remove the unnecessary filter and simply use the current set
(e.g., val real = this.toSet() or val real = this) when computing real, then
keep the existing size comparison against DiscoveryPlatform.selectablePlatforms
to decide whether to return emptySet() or real; update the body of normalize()
accordingly to eliminate the no-op filter of DiscoveryPlatform.All.
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.kt`:
- Around line 744-758: The conditional Icon rendering makes the Text shift when
isSelected toggles; always reserve the icon space instead: render the Icon
unconditionally (the same Icon call in HomeRoot.kt) but set its visibility via
Modifier.alpha(if (isSelected) 1f else 0f) or wrap it in a fixed-width
container/Spacer matching Modifier.size(20.dp) so the Text (the Text(...) call)
keeps a stable position when isSelected changes.
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeViewModel.kt`:
- Around line 320-340: The forEach over topics in HomeViewModel.kt currently
calls homeRepository.searchByTopic(...).collect which, if it throws for one
topic, aborts the loop and skips remaining topics; wrap each per-topic
search/collect in its own try-catch (or use runCatching) around the call to
homeRepository.searchByTopic and the mapping via mapReposToUi so that any
exception is caught, logged (or handled) and the loop continues, while
preserving the existing _state.update behavior for successful results.
- Around line 162-172: In HomeViewModel, avoid launching the background async
job unconditionally: the targetPlatformsDeferred created via
viewModelScope.async is started even when platforms is supplied; either create
the deferred with viewModelScope.async(start = CoroutineStart.LAZY) and only
call await() when platforms == null, or move the viewModelScope.async call
inside the branch where platforms is null so it only starts when needed; update
references to targetPlatformsDeferred.await() used in the launch block (and keep
logger/variable names: targetPlatformsDeferred, platforms, targetPlatforms) so
the deferred is only started/awaited when required.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa687377-e1c4-4505-a39a-fcbc3c96c443
⛔ Files ignored due to path filters (1)
feature/home/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (11)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/dao/InstalledAppDao.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/DiscoveryPlatform.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/TweaksRepository.ktfeature/home/data/src/commonMain/kotlin/zed/rainxch/home/data/repository/HomeRepositoryImpl.ktfeature/home/domain/src/commonMain/kotlin/zed/rainxch/home/domain/repository/HomeRepository.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeAction.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeState.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeViewModel.kt
Comment on lines
+152
to
165
| override fun getDiscoveryPlatforms(): Flow<Set<DiscoveryPlatform>> = | ||
| preferences.data.map { prefs -> | ||
| val platform = prefs[DISCOVERY_PLATFORM_KEY] | ||
| DiscoveryPlatform.fromName(platform) | ||
| val stored = prefs[DISCOVERY_PLATFORMS_KEY] | ||
| if (stored != null) { | ||
| stored | ||
| .mapNotNull { name -> | ||
| DiscoveryPlatform.entries.find { it.name == name && it != DiscoveryPlatform.All } | ||
| }.toSet() | ||
| } else { | ||
| // Legacy single-platform key migration: map old `All` to empty set. | ||
| val legacy = prefs[DISCOVERY_PLATFORM_KEY]?.let { DiscoveryPlatform.fromName(it) } | ||
| if (legacy != null && legacy != DiscoveryPlatform.All) setOf(legacy) else emptySet() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Migration never graduates to the new key until the user explicitly saves
The legacy fallback branch (lines 160–163) reads DISCOVERY_PLATFORM_KEY on every emission but never back-fills DISCOVERY_PLATFORMS_KEY. For existing users whose old key is set, every cold-start read of getDiscoveryPlatforms() will continue hitting the legacy path indefinitely — until setDiscoveryPlatforms is explicitly called (e.g. when the user changes their platform selection).
Consider performing a one-shot write-through when migrating, so subsequent reads go straight to the new key:
💡 Proposed fix — write-through on first migration
} else {
// Legacy single-platform key migration: map old `All` to empty set.
val legacy = prefs[DISCOVERY_PLATFORM_KEY]?.let { DiscoveryPlatform.fromName(it) }
- if (legacy != null && legacy != DiscoveryPlatform.All) setOf(legacy) else emptySet()
+ val migrated = if (legacy != null && legacy != DiscoveryPlatform.All) setOf(legacy) else emptySet()
+ // Back-fill so future reads use the new key directly.
+ // Note: DataStore.data.map is read-only; perform the write in a side-channel
+ // or remove this comment if you intentionally prefer lazy migration.
+ migrated
}If you intentionally prefer the lazy approach, a short comment explaining that
setDiscoveryPlatformsis always called before the legacy key matters in practice would be sufficient.
📝 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
| override fun getDiscoveryPlatforms(): Flow<Set<DiscoveryPlatform>> = | |
| preferences.data.map { prefs -> | |
| val platform = prefs[DISCOVERY_PLATFORM_KEY] | |
| DiscoveryPlatform.fromName(platform) | |
| val stored = prefs[DISCOVERY_PLATFORMS_KEY] | |
| if (stored != null) { | |
| stored | |
| .mapNotNull { name -> | |
| DiscoveryPlatform.entries.find { it.name == name && it != DiscoveryPlatform.All } | |
| }.toSet() | |
| } else { | |
| // Legacy single-platform key migration: map old `All` to empty set. | |
| val legacy = prefs[DISCOVERY_PLATFORM_KEY]?.let { DiscoveryPlatform.fromName(it) } | |
| if (legacy != null && legacy != DiscoveryPlatform.All) setOf(legacy) else emptySet() | |
| } | |
| } | |
| override fun getDiscoveryPlatforms(): Flow<Set<DiscoveryPlatform>> = | |
| preferences.data.map { prefs -> | |
| val stored = prefs[DISCOVERY_PLATFORMS_KEY] | |
| if (stored != null) { | |
| stored | |
| .mapNotNull { name -> | |
| DiscoveryPlatform.entries.find { it.name == name && it != DiscoveryPlatform.All } | |
| }.toSet() | |
| } else { | |
| // Legacy single-platform key migration: map old `All` to empty set. | |
| val legacy = prefs[DISCOVERY_PLATFORM_KEY]?.let { DiscoveryPlatform.fromName(it) } | |
| val migrated = if (legacy != null && legacy != DiscoveryPlatform.All) setOf(legacy) else emptySet() | |
| // Back-fill so future reads use the new key directly. | |
| // Note: DataStore.data.map is read-only; perform the write in a side-channel | |
| // or remove this comment if you intentionally prefer lazy migration. | |
| migrated | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.kt`
around lines 152 - 165, The legacy-branch in getDiscoveryPlatforms() reads
DISCOVERY_PLATFORM_KEY but never writes DISCOVERY_PLATFORMS_KEY, so migration
never completes; modify the branch so that when stored == null and a legacy
non-All value is found, perform a one-shot write-through to backfill
DISCOVERY_PLATFORMS_KEY (using preferences.edit to set the new key to a set
containing the migrated platform) and then return that set; ensure the write
only happens when migrating (stored == null && legacy != All) to avoid loops and
leave the rest of the mapping behavior unchanged (references:
getDiscoveryPlatforms(), DISCOVERY_PLATFORMS_KEY, DISCOVERY_PLATFORM_KEY,
setDiscoveryPlatforms).
…repository/HomeRepositoryImpl.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Summary by CodeRabbit
Bug Fixes
New Features