Add from starred picker (#444 ask 2/3)#503
Merged
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a "starred picker" feature: new UI, state, actions/events, ViewModel, navigation destination and wiring, DI registration, localized strings, and release-note updates to surface APK-shipping repos from a user’s GitHub stars and navigate into details/install flow. ChangesStarred Picker Feature
Sequence DiagramsequenceDiagram
participant User
participant AppsScreen
participant Navigation
participant StarredPickerUI as StarredPickerRoot
participant VM as StarredPickerViewModel
participant GitHub
participant LocalRepo as InstalledAppsRepository/LocalDB
User->>AppsScreen: Tap "Add from starred"
AppsScreen->>Navigation: navigate(StarredPickerScreen)
Navigation->>StarredPickerUI: render
StarredPickerUI->>VM: collect state / observe events
VM->>LocalRepo: check auth + load installed apps
alt not authenticated
VM-->>StarredPickerUI: phase = Empty
StarredPickerUI->>User: show "sign in required"
else authenticated
VM->>GitHub: sync starred repos / load cached stars
VM->>LocalRepo: build tracked keys
VM-->>StarredPickerUI: candidates, phase = ScanningReleases
loop per candidate
VM->>GitHub: fetch latest release
GitHub-->>VM: release assets
VM-->>StarredPickerUI: update candidate.hasApkRelease / progress
end
VM-->>StarredPickerUI: phase = Ready
User->>StarredPickerUI: click candidate
StarredPickerUI->>VM: OnCandidateClick
VM-->>Navigation: NavigateToDetails(repoId, owner, repo)
Navigation->>User: navigate to DetailsScreen
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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: 5
🧹 Nitpick comments (3)
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt (1)
240-241: 💤 Low valueUnreachable branch —
OnAddFromStarredClickis fully handled inAppsRootbefore reaching the ViewModel.
AppsRoot.onActioninterceptsAppsAction.OnAddFromStarredClickin its ownwhenblock (callingonNavigateToStarredPicker()) and only theelsebranch forwards toviewModel.onAction(). This mirrors the pre-existing emptyOnNavigateBackClick -> {}at line 237.If this is intentional for exhaustiveness/documentation purposes, consider a comment clarifying that; otherwise the branch can be removed since the sealed
whenhere is a statement (not an expression) and doesn't require it.♻️ Optional: remove the unreachable branch
- AppsAction.OnAddFromStarredClick -> { - } - is AppsAction.OnSearchChange -> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt` around lines 240 - 241, The empty AppsAction.OnAddFromStarredClick branch in AppsViewModel is unreachable because AppsRoot.onAction intercepts that action (calling onNavigateToStarredPicker()) before forwarding other actions to viewModel.onAction; remove the AppsAction.OnAddFromStarredClick -> { } case from AppsViewModel.onAction to simplify the sealed when, or if you want to keep it for documentation/exhaustiveness add a one-line comment referencing AppsRoot.onAction handling and mark it intentionally unreachable; update the AppsViewModel.onAction method accordingly to avoid dead branches.feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerViewModel.kt (1)
1-2: 💤 Low valueRemove unused
@OptIn(ExperimentalTime::class)annotation.The
ExperimentalTimeopt-in is declared but no experimental time APIs are used in this file. This appears to be leftover from a previous iteration.♻️ Suggested cleanup
-@file:OptIn(ExperimentalTime::class) - package zed.rainxch.apps.presentation.starredAlso remove the unused import:
-import kotlin.time.ExperimentalTime🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerViewModel.kt` around lines 1 - 2, Remove the unused opt-in and related import: delete the `@OptIn`(ExperimentalTime::class) annotation at the top of StarredPickerViewModel.kt and also remove the unused import for kotlin.time.ExperimentalTime (or any kotlin.time.* import) so the file no longer declares an unnecessary ExperimentalTime opt-in.feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerRoot.kt (1)
69-69: ⚡ Quick winConsider using
collectAsStateWithLifecycle()for lifecycle-aware collection.Other screens in this codebase (e.g.,
AppNavigation.ktlines 70, 74) usecollectAsStateWithLifecycle()fromandroidx.lifecycle.composeto pause collection when the app is in the background. Using plaincollectAsState()may cause unnecessary work when the composable isn't visible.♻️ Suggested change
-import androidx.compose.runtime.collectAsState +import androidx.lifecycle.compose.collectAsStateWithLifecycle- val state by viewModel.state.collectAsState() + val state by viewModel.state.collectAsStateWithLifecycle()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerRoot.kt` at line 69, Replace the direct StateFlow collection call using viewModel.state.collectAsState() in StarredPickerRoot.kt with the lifecycle-aware collectAsStateWithLifecycle() to pause collection when the composable is not visible: update the expression val state by viewModel.state.collectAsState() to use collectAsStateWithLifecycle(), add the necessary import from androidx.lifecycle.compose.collectAsStateWithLifecycle, and ensure the module has the lifecycle-compose dependency so collection is lifecycle-aware.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/components/StarredCandidateRow.kt`:
- Line 92: Replace the Spacer usage in StarredCandidateRow (the
Spacer(Modifier.padding(top = 2.dp)) instance) so it uses a sizing modifier
instead of padding — change it to use Modifier.height(2.dp) to produce the
intended 2dp vertical gap; ensure the Spacer import and surrounding composable
remain unchanged.
- Around line 44-51: The accessibility label in StarredCandidateRow (a11yLabel)
uses hardcoded English fragments; replace those literals with localized string
resources by adding keys like starred_picker_cd_ships_apk,
starred_picker_cd_already_tracked, and starred_picker_cd_latest (with a
placeholder for the tag) in strings.xml for all locales, then call
stringResource(...) inside the `@Composable` to append the localized texts when
candidate.hasApkRelease, candidate.isAlreadyTracked, and
candidate.latestReleaseTag != null (use the placeholder-based resource to inject
latestReleaseTag).
- Around line 141-153: The Badge composable applies clip but no background fill
so the pill isn't visible; update the Row modifier in Badge to include a
background using the same RoundedCornerShape and a translucent variant of the
passed color (e.g., color.copy(alpha = 0.15f)) before or after clip, so the
badge has a filled pill appearance, and add the required import for
androidx.compose.foundation.background; locate the Badge function to change its
Modifier chain and keep Icon/Text tinting as-is.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerRoot.kt`:
- Around line 97-101: The Icon used in StarredPickerRoot.kt sets
contentDescription = null which prevents screen readers from announcing the back
button; replace the null with a localized string via stringResource (e.g.,
contentDescription = stringResource(R.string.back_button_description) or
R.string.back) and add the required import for
androidx.compose.ui.res.stringResource and the new string resource entry
(back_button_description) in your resources so the navigation icon is accessible
to assistive technologies.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerViewModel.kt`:
- Around line 187-191: The catch block that handles Exception when updating
_state (inside the scan loop updating scanProgress via current.copy(scanProgress
= ++processed)) is swallowing errors; modify the catch to record or log the
exception (use the project's logger if available, or add the exception message
to state—e.g., a debug or error field or increment a failure counter on the
ViewModel state) while still incrementing processed so behavior is unchanged;
update the catch around the code handling processed and _state.update to include
the exception information (e.g., call logger.error(...) or update
current.copy(failedCount = current.failedCount + 1, lastError = e.message)) so
failures are preserved for debugging.
---
Nitpick comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Around line 240-241: The empty AppsAction.OnAddFromStarredClick branch in
AppsViewModel is unreachable because AppsRoot.onAction intercepts that action
(calling onNavigateToStarredPicker()) before forwarding other actions to
viewModel.onAction; remove the AppsAction.OnAddFromStarredClick -> { } case from
AppsViewModel.onAction to simplify the sealed when, or if you want to keep it
for documentation/exhaustiveness add a one-line comment referencing
AppsRoot.onAction handling and mark it intentionally unreachable; update the
AppsViewModel.onAction method accordingly to avoid dead branches.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerRoot.kt`:
- Line 69: Replace the direct StateFlow collection call using
viewModel.state.collectAsState() in StarredPickerRoot.kt with the
lifecycle-aware collectAsStateWithLifecycle() to pause collection when the
composable is not visible: update the expression val state by
viewModel.state.collectAsState() to use collectAsStateWithLifecycle(), add the
necessary import from androidx.lifecycle.compose.collectAsStateWithLifecycle,
and ensure the module has the lifecycle-compose dependency so collection is
lifecycle-aware.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerViewModel.kt`:
- Around line 1-2: Remove the unused opt-in and related import: delete the
`@OptIn`(ExperimentalTime::class) annotation at the top of
StarredPickerViewModel.kt and also remove the unused import for
kotlin.time.ExperimentalTime (or any kotlin.time.* import) so the file no longer
declares an unnecessary ExperimentalTime opt-in.
🪄 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: ae7099b0-c8bf-443b-8a35-526397b8d0c3
📒 Files selected for processing (26)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/16.jsoncore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsAction.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerAction.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerEvent.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerState.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/StarredPickerViewModel.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/components/StarredCandidateRow.kt
Comment on lines
+44
to
+51
| val a11yLabel = buildString { | ||
| append(candidate.owner) | ||
| append(" / ") | ||
| append(candidate.name) | ||
| if (candidate.hasApkRelease) append(", ships APK") | ||
| if (candidate.isAlreadyTracked) append(", already tracked") | ||
| candidate.latestReleaseTag?.let { append(", latest ").append(it) } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Accessibility label contains hardcoded English strings — localize them.
The strings ", ships APK", ", already tracked", and ", latest " are baked in as English literals. Since this is a @Composable function, stringResource is callable here. These should use localized resources so TalkBack announces the correct language for all 13 supported locales.
♻️ Proposed fix
Add dedicated string resources (e.g., starred_picker_cd_ships_apk, starred_picker_cd_already_tracked, starred_picker_cd_latest) to strings.xml and all locale files, then:
+ val shipsApkLabel = stringResource(Res.string.starred_picker_cd_ships_apk)
+ val trackedLabel = stringResource(Res.string.starred_picker_cd_already_tracked)
+ val latestLabel = stringResource(Res.string.starred_picker_cd_latest)
val a11yLabel = buildString {
append(candidate.owner)
append(" / ")
append(candidate.name)
- if (candidate.hasApkRelease) append(", ships APK")
- if (candidate.isAlreadyTracked) append(", already tracked")
- candidate.latestReleaseTag?.let { append(", latest ").append(it) }
+ if (candidate.hasApkRelease) append(", $shipsApkLabel")
+ if (candidate.isAlreadyTracked) append(", $trackedLabel")
+ candidate.latestReleaseTag?.let { append(", $latestLabel ").append(it) }
}As per coding guidelines, "Localize all user-facing strings using the 13-language localization system provided by core/presentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/starred/components/StarredCandidateRow.kt`
around lines 44 - 51, The accessibility label in StarredCandidateRow (a11yLabel)
uses hardcoded English fragments; replace those literals with localized string
resources by adding keys like starred_picker_cd_ships_apk,
starred_picker_cd_already_tracked, and starred_picker_cd_latest (with a
placeholder for the tag) in strings.xml for all locales, then call
stringResource(...) inside the `@Composable` to append the localized texts when
candidate.hasApkRelease, candidate.isAlreadyTracked, and
candidate.latestReleaseTag != null (use the placeholder-based resource to inject
latestReleaseTag).
rainxchzed
force-pushed
the
feat/444-add-from-starred
branch
from
244ccb0 to
6d8e199
Compare
7 tasks
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.
Second of three PRs for #444. Surfaces APK-shipping repos from the user's GitHub stars and routes them into the standard install flow. See plan in `roadmap/444_B_STARRED_REPOS_SYNC.md`.
What's in here
What's deferred
Plan B originally proposed bulk one-tap "Add 30 apps". The `InstalledApp` Room schema requires `packageName` as the primary key, and starred-repo metadata doesn't carry a package name (the repo's APK might pin a different applicationId, the user might not have it installed yet, etc.). True bulk-add without an installed APK requires a tracking-only schema with synthetic-key support — bigger change than this PR's scope.
This phase 1 ships the discovery surface today: scan starred for APKs, surface them, route to Details for normal install/track. Phase 2 (true bulk-track without local install) is a follow-up tracked separately once the schema can support it.
The UX is honest about the constraint — per-row tap goes through the existing flow, not a fake one-tap-add. No silent failures, no half-tracked rows.
Test plan
Summary by CodeRabbit
New Features
Documentation / Localization