feat(e15): multi-OS release picker β download installers for other platforms#588
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. π βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: π Files selected for processing (1)
π§ Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a persisted "show all platforms" toggle and cross-platform release-assets picker that groups assets by detected platform; current-platform assets remain installable, others download via browser for transfer. Includes repository APIs, platform detection utility, ViewModel wiring, UI components, header integration, release notes, and localized strings. ChangesMulti-OS Release Asset Picker with Platform Toggling
Estimated code review effortπ― 4 (Complex) | β±οΈ ~45 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. Comment |
Greptile SummaryImplements the multi-OS release asset picker (E15): a persisted "Show all platforms" toggle on the asset-picker sheet that groups release installers by platform (Android / Windows / macOS / Linux) and routes non-current-platform downloads through the browser.
Confidence Score: 5/5Safe to merge β all core logic paths (install, toggle persistence, cross-platform routing) are correct and follow established patterns. The DataStore integration, ViewModel action handling, and Compose memoisation are all implemented correctly. The only gap is a dead string resource (saved_for_transfer_hint) that was described in the PR as a visible hint row but was never wired to ReleaseAssetItem; this is a UX omission rather than a functional defect. ReleaseAssetsPicker.kt β the saved_for_transfer_hint string is defined across all locale files but the per-asset hint row is not rendered. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant H as Header.kt
participant P as ReleaseAssetsPicker
participant VM as DetailsViewModel
participant TR as TweaksRepository
H->>H: remember(selectedRelease) β crossPlatformAssets
H->>P: showAllPlatforms, crossPlatformAssets, assetsList
U->>P: Open picker sheet
P->>P: "isPickerEnabled = assetsList||crossPlatformAssets non-empty"
U->>P: Toggle Show all platforms
P->>VM: OnToggleShowAllPlatforms(enabled)
VM->>TR: setShowAllPlatforms(enabled)
TR-->>VM: "Flow<Boolean> emits"
VM-->>H: state.showAllPlatforms updated
alt "showAllPlatforms=true and groups non-empty"
P->>P: Render PlatformSectionCard per DiscoveryPlatform
U->>P: Tap current-platform asset
P->>VM: SelectDownloadAsset(asset)
U->>P: Tap other-platform asset
P->>VM: OnDownloadForTransfer(assetUrl)
VM->>VM: helper.openUrl(assetUrl)
else "showAllPlatforms=false or no classified assets"
P->>P: Render assetsList (current-platform only)
U->>P: Tap asset
P->>VM: SelectDownloadAsset(asset)
end
Reviews (4): Last reviewed commit: "fix(e15): fall back to assetsList when t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (1)
core/presentation/src/commonMain/composeResources/values-ko/strings-ko.xml (1)
1151-1151: β‘ Quick winConsider a more concise Korean phrasing for better readability.
The current translation is grammatically correct and understandable, but it's slightly verbose. A more natural Korean phrasing would improve the user experience.
π Suggested alternative phrasing
- <string name="saved_for_transfer_hint">λ€λ₯Έ νλ«νΌ β μ μ‘μ© μ μ₯μ μν΄ λΈλΌμ°μ μμ μ΄λ¦½λλ€</string> + <string name="saved_for_transfer_hint">λ€λ₯Έ νλ«νΌ β λΈλΌμ°μ μμ λ€μ΄λ‘λνμ¬ μ μ‘</string>This makes the hint more concise while preserving the meaning: "Other platform β Download in browser to transfer."
π€ 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/presentation/src/commonMain/composeResources/values-ko/strings-ko.xml` at line 1151, Replace the verbose Korean value for the string resource saved_for_transfer_hint with a more concise, natural phrasing; update the text from "λ€λ₯Έ νλ«νΌ β μ μ‘μ© μ μ₯μ μν΄ λΈλΌμ°μ μμ μ΄λ¦½λλ€" to a shorter form such as "λ€λ₯Έ νλ«νΌ β μ μ‘νλ €λ©΄ λΈλΌμ°μ μμ λ€μ΄λ‘λνμΈμ" in the strings-ko.xml entry for saved_for_transfer_hint.
π€ 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt`:
- Around line 76-79: The current derivedStateOf in the remember block for
isPickerEnabled only enables the picker when showAllPlatforms is true and
crossPlatformAssets exists, or when showAllPlatforms is false and assetsList
exists; change the predicate so the picker is enabled whenever either assetsList
or crossPlatformAssets is non-empty (i.e., use assetsList.isNotEmpty() ||
crossPlatformAssets.isNotEmpty()) inside the remember/derivedStateOf for
isPickerEnabled in ReleaseAssetsPicker.kt to allow the multi-OS flow when local
installers are missing but other-platform installers exist.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 497-502: The current handler for
DetailsAction.OnToggleShowAllPlatforms flips based on
_state.value.showAllPlatforms which can race on rapid toggles; change the action
to carry the explicit enabled boolean
(DetailsAction.OnToggleShowAllPlatforms(enabled)) and in the ViewModel use that
enabled value directly when calling
tweaksRepository.setShowAllPlatforms(enabled) inside the existing
viewModelScope.launch (preserving the CancellationException catch), and update
the UI to call onAction(DetailsAction.OnToggleShowAllPlatforms(checked)) from
its onCheckedChange so the persisted value matches the user's intent.
---
Nitpick comments:
In `@core/presentation/src/commonMain/composeResources/values-ko/strings-ko.xml`:
- Line 1151: Replace the verbose Korean value for the string resource
saved_for_transfer_hint with a more concise, natural phrasing; update the text
from "λ€λ₯Έ νλ«νΌ β μ μ‘μ© μ μ₯μ μν΄ λΈλΌμ°μ μμ μ΄λ¦½λλ€" to a shorter form such as "λ€λ₯Έ νλ«νΌ β μ μ‘νλ €λ©΄
λΈλΌμ°μ μμ λ€μ΄λ‘λνμΈμ" in the strings-ko.xml entry for saved_for_transfer_hint.
πͺ 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: 316c49df-1c96-4567-88a0-903b4b127374
π Files selected for processing (22)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/TweaksRepository.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/util/AssetPlatform.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/17.jsoncore/presentation/src/commonMain/composeResources/values-ar/strings-ar.xmlcore/presentation/src/commonMain/composeResources/values-bn/strings-bn.xmlcore/presentation/src/commonMain/composeResources/values-es/strings-es.xmlcore/presentation/src/commonMain/composeResources/values-fr/strings-fr.xmlcore/presentation/src/commonMain/composeResources/values-hi/strings-hi.xmlcore/presentation/src/commonMain/composeResources/values-it/strings-it.xmlcore/presentation/src/commonMain/composeResources/values-ja/strings-ja.xmlcore/presentation/src/commonMain/composeResources/values-ko/strings-ko.xmlcore/presentation/src/commonMain/composeResources/values-pl/strings-pl.xmlcore/presentation/src/commonMain/composeResources/values-ru/strings-ru.xmlcore/presentation/src/commonMain/composeResources/values-tr/strings-tr.xmlcore/presentation/src/commonMain/composeResources/values-zh-rCN/strings-zh-rCN.xmlcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsAction.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsState.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Header.kt
Contributor
There was a problem hiding this comment.
β»οΈ Duplicate comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt (1)
79-83:β οΈ Potential issue | π Major | β‘ Quick winPicker stays disabled when only cross-platform assets exist.
When
showAllPlatformsisfalseandassetsListis empty, the OutlinedCard is disabled β butcrossPlatformAssetsmay have installers for other OSes. Users in that state cannot open the sheet to flip the toggle on, defeating the multi-OS flow exactly when it's needed. DecoupleisPickerEnabledfromshowAllPlatforms.π§ Proposed fix
- val isPickerEnabled by remember(assetsList, crossPlatformAssets, showAllPlatforms) { + val isPickerEnabled by remember(assetsList, crossPlatformAssets) { derivedStateOf { - if (showAllPlatforms) crossPlatformAssets.isNotEmpty() else assetsList.isNotEmpty() + assetsList.isNotEmpty() || crossPlatformAssets.isNotEmpty() } }π€ 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/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt` around lines 79 - 83, The computed state isPickerEnabled incorrectly depends on showAllPlatforms, causing the picker to stay disabled when assetsList is empty but crossPlatformAssets contains installers; update the derivedStateOf in the remember block (isPickerEnabled) to return true if either assetsList.isNotEmpty() or crossPlatformAssets.isNotEmpty(), and adjust the remember keys to still include assetsList and crossPlatformAssets (no need to depend on showAllPlatforms) so the OutlinedCard can open whenever any assets exist and the sheet can let users toggle showAllPlatforms.
π§Ή Nitpick comments (2)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt (2)
269-320: π€ Low valueHoist per-recomposition work into
rememberand consider importing the fully-qualified symbols.
groups,installableIds, andsectionOrderare rebuilt on every recomposition of the sheet (each animation frame, each scroll-state change of the LazyColumn parent, etc.). They're pure functions ofcrossPlatformAssetsandassetsList, so wrapping them inremember(crossPlatformAssets, assetsList) { β¦ }is a cheap win. While here, importingDiscoveryPlatformandassetPlatformOfat the top of the file improves readability over inline FQNs.β»οΈ Proposed refactor sketch
+import zed.rainxch.core.domain.model.DiscoveryPlatform +import zed.rainxch.core.domain.util.assetPlatformOf @@ - if (showAllPlatforms) { - ... - val groups: Map<zed.rainxch.core.domain.model.DiscoveryPlatform, List<GithubAsset>> = - crossPlatformAssets - .groupBy { zed.rainxch.core.domain.util.assetPlatformOf(it.name) } - .filterKeys { it != null } - .mapKeys { it.key!! } + if (showAllPlatforms) { + val groups = remember(crossPlatformAssets) { + crossPlatformAssets + .groupBy { assetPlatformOf(it.name) } + .mapNotNull { (k, v) -> k?.let { it to v } } + .toMap() + } + val installableIds = remember(assetsList) { assetsList.mapTo(HashSet()) { it.id } }π€ 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/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt` around lines 269 - 320, The grouping and ordering work (variables groups, installableIds, sectionOrder derived from crossPlatformAssets and assetsList) are recomputed on every recompositionβwrap those computations in a remember(crossPlatformAssets, assetsList) { ... } block so they are only recalculated when inputs change, and move the assetPlatformOf and DiscoveryPlatform FQNs to top-level imports to simplify the code; specifically, compute val (groups, installableIds, sectionOrder) = remember(crossPlatformAssets, assetsList) { /* groupBy(assetPlatformOf), filter/map keys, build installableIds set, and build sectionOrder list */ } and keep the rest of the UI logic using these remembered values.
223-255: β‘ Quick winUse
Modifier.toggleablewithRole.Switchfor the toggle row.The Row currently combines
.clickable { onToggleShowAllPlatforms() }with aSwitchthat has its ownonCheckedChange. Screen readers see two distinct actionable nodes instead of one switch control, and the row's click semantics don't expose checked state. The idiomatic Material3 pattern is to puttoggleableon the row withrole = Role.Switchand passonCheckedChange = nullto the Switch so it inherits the row's semantics.β»οΈ Proposed refactor
+import androidx.compose.foundation.selection.toggleable +import androidx.compose.ui.semantics.Role @@ - Row( - modifier = - Modifier - .clickable(onClick = onToggleShowAllPlatforms) - .padding(horizontal = 16.dp, vertical = 10.dp), - verticalAlignment = Alignment.CenterVertically, - ) { + Row( + modifier = + Modifier + .toggleable( + value = showAllPlatforms, + role = Role.Switch, + onValueChange = { onToggleShowAllPlatforms() }, + ) + .padding(horizontal = 16.dp, vertical = 10.dp), + verticalAlignment = Alignment.CenterVertically, + ) { @@ - androidx.compose.material3.Switch( - checked = showAllPlatforms, - onCheckedChange = { onToggleShowAllPlatforms() }, - ) + androidx.compose.material3.Switch( + checked = showAllPlatforms, + onCheckedChange = null, + )π€ 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/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt` around lines 223 - 255, The Row uses Modifier.clickable plus a Switch, which creates duplicate actionable nodes and hides checked semantics; replace Modifier.clickable with Modifier.toggleable(value = showAllPlatforms, onValueChange = { onToggleShowAllPlatforms() }, role = Role.Switch) on the Row (the component containing the Icon/Text/Switch) and update the Switch inside ReleaseAssetsPicker to pass onCheckedChange = null and keep checked = showAllPlatforms so the row owns the accessibility semantics and state toggling.
π€ 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt`:
- Around line 79-83: The computed state isPickerEnabled incorrectly depends on
showAllPlatforms, causing the picker to stay disabled when assetsList is empty
but crossPlatformAssets contains installers; update the derivedStateOf in the
remember block (isPickerEnabled) to return true if either
assetsList.isNotEmpty() or crossPlatformAssets.isNotEmpty(), and adjust the
remember keys to still include assetsList and crossPlatformAssets (no need to
depend on showAllPlatforms) so the OutlinedCard can open whenever any assets
exist and the sheet can let users toggle showAllPlatforms.
---
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt`:
- Around line 269-320: The grouping and ordering work (variables groups,
installableIds, sectionOrder derived from crossPlatformAssets and assetsList)
are recomputed on every recompositionβwrap those computations in a
remember(crossPlatformAssets, assetsList) { ... } block so they are only
recalculated when inputs change, and move the assetPlatformOf and
DiscoveryPlatform FQNs to top-level imports to simplify the code; specifically,
compute val (groups, installableIds, sectionOrder) =
remember(crossPlatformAssets, assetsList) { /* groupBy(assetPlatformOf),
filter/map keys, build installableIds set, and build sectionOrder list */ } and
keep the rest of the UI logic using these remembered values.
- Around line 223-255: The Row uses Modifier.clickable plus a Switch, which
creates duplicate actionable nodes and hides checked semantics; replace
Modifier.clickable with Modifier.toggleable(value = showAllPlatforms,
onValueChange = { onToggleShowAllPlatforms() }, role = Role.Switch) on the Row
(the component containing the Icon/Text/Switch) and update the Switch inside
ReleaseAssetsPicker to pass onCheckedChange = null and keep checked =
showAllPlatforms so the row owns the accessibility semantics and state toggling.
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9986a6a7-edc5-406e-b00f-ec73c52502d5
π Files selected for processing (14)
core/presentation/src/commonMain/composeResources/values-ar/strings-ar.xmlcore/presentation/src/commonMain/composeResources/values-bn/strings-bn.xmlcore/presentation/src/commonMain/composeResources/values-es/strings-es.xmlcore/presentation/src/commonMain/composeResources/values-fr/strings-fr.xmlcore/presentation/src/commonMain/composeResources/values-hi/strings-hi.xmlcore/presentation/src/commonMain/composeResources/values-it/strings-it.xmlcore/presentation/src/commonMain/composeResources/values-ja/strings-ja.xmlcore/presentation/src/commonMain/composeResources/values-ko/strings-ko.xmlcore/presentation/src/commonMain/composeResources/values-pl/strings-pl.xmlcore/presentation/src/commonMain/composeResources/values-ru/strings-ru.xmlcore/presentation/src/commonMain/composeResources/values-tr/strings-tr.xmlcore/presentation/src/commonMain/composeResources/values-zh-rCN/strings-zh-rCN.xmlcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt
β Files skipped from review due to trivial changes (2)
- core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml
- core/presentation/src/commonMain/composeResources/values-ko/strings-ko.xml
π§ Files skipped from review as they are similar to previous changes (9)
- core/presentation/src/commonMain/composeResources/values-pl/strings-pl.xml
- core/presentation/src/commonMain/composeResources/values-tr/strings-tr.xml
- core/presentation/src/commonMain/composeResources/values-ru/strings-ru.xml
- core/presentation/src/commonMain/composeResources/values-it/strings-it.xml
- core/presentation/src/commonMain/composeResources/values-ar/strings-ar.xml
- core/presentation/src/commonMain/composeResources/values/strings.xml
- core/presentation/src/commonMain/composeResources/values-es/strings-es.xml
- core/presentation/src/commonMain/composeResources/values-zh-rCN/strings-zh-rCN.xml
- core/presentation/src/commonMain/composeResources/values-ja/strings-ja.xml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
π§Ή Nitpick comments (2)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsAction.kt (1)
170-177: β‘ Quick winConsider including
assetNameparameter for better observability.The existing
DownloadAssetaction (lines 33-37) includesassetNamealongside the URL. IncludingassetNamehere would improve log messages, error diagnostics, and analytics when the transfer-download flow encounters issues, especially since the ViewModel logs failures for this action.π Suggested enhancement
data class OnDownloadForTransfer( val assetUrl: String, + val assetName: String, ) : DetailsActionπ€ 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/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsAction.kt` around lines 170 - 177, The OnDownloadForTransfer data class lacks an assetName field which reduces observability compared to the existing DownloadAsset action; update the DetailsAction sealed types by adding a val assetName: String parameter to the OnDownloadForTransfer data class (mirroring DownloadAsset's shape), update all call sites and ViewModel logging/analytics that construct or consume OnDownloadForTransfer to pass and use assetName, and adjust any serializers/tests that reference DetailsAction/OnDownloadForTransfer accordingly so logs and error diagnostics include the asset name.feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt (1)
229-261: β‘ Quick winUse
Modifier.toggleable(role = Role.Switch)instead ofclickable+Switch.onCheckedChange.The row currently exposes two interactive nodes to accessibility services: the
clickableRow (no role) and theSwitchitself. TalkBack/Switch Access will see them as separate targets and double-announce the control. The Material3-recommended pattern is to drive the toggle from the row withtoggleable(role = Role.Switch)and make the Switch a non-interactive visual indicator.β»οΈ Proposed refactor
+import androidx.compose.foundation.selection.toggleable +import androidx.compose.ui.semantics.Role @@ Surface( shape = RoundedCornerShape(20.dp), color = MaterialTheme.colorScheme.surfaceContainerHigh, modifier = Modifier .fillMaxWidth() .padding(horizontal = 16.dp, vertical = 8.dp), ) { Row( modifier = Modifier - .clickable(onClick = { onToggleShowAllPlatforms(!showAllPlatforms) }) + .toggleable( + value = showAllPlatforms, + onValueChange = onToggleShowAllPlatforms, + role = Role.Switch, + ) .padding(horizontal = 16.dp, vertical = 10.dp), verticalAlignment = Alignment.CenterVertically, ) { @@ androidx.compose.material3.Switch( checked = showAllPlatforms, - onCheckedChange = onToggleShowAllPlatforms, + onCheckedChange = null, ) } }π€ 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/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt` around lines 229 - 261, The Row that currently uses Modifier.clickable(...) and a live androidx.compose.material3.Switch should be converted to a single toggleable control: replace Modifier.clickable in the Row with Modifier.toggleable(checked = showAllPlatforms, onValueChange = onToggleShowAllPlatforms, role = Role.Switch) and make the Switch non-interactive (remove its onCheckedChange and keep checked = showAllPlatforms only) so the Row drives the toggle; update imports to include androidx.compose.foundation.selection.toggleable and androidx.compose.ui.semantics.Role as needed and ensure the unique identifiers (the Row block, onToggleShowAllPlatforms, showAllPlatforms, and the Switch component) are adjusted accordingly.
π€ 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
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt`:
- Around line 282-293: When showAllPlatforms is true the UI currently shows the
empty-state when groups.isEmpty() even though installableAssets (derived via
installer.isAssetInstallable(asset.name) from assetsList) may contain items;
change the conditional to show the "no assets" message only when both groups and
the installable assets set are empty (e.g. replace the check if
(groups.isEmpty()) with if (groups.isEmpty() && installableAssets.isEmpty()) or
equivalent), keeping references to installer.isAssetInstallable,
assetPlatformOf, assetsList, installableAssets and crossPlatformAssets to ensure
assets without platform mappings still surface.
---
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.kt`:
- Around line 229-261: The Row that currently uses Modifier.clickable(...) and a
live androidx.compose.material3.Switch should be converted to a single
toggleable control: replace Modifier.clickable in the Row with
Modifier.toggleable(checked = showAllPlatforms, onValueChange =
onToggleShowAllPlatforms, role = Role.Switch) and make the Switch
non-interactive (remove its onCheckedChange and keep checked = showAllPlatforms
only) so the Row drives the toggle; update imports to include
androidx.compose.foundation.selection.toggleable and
androidx.compose.ui.semantics.Role as needed and ensure the unique identifiers
(the Row block, onToggleShowAllPlatforms, showAllPlatforms, and the Switch
component) are adjusted accordingly.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsAction.kt`:
- Around line 170-177: The OnDownloadForTransfer data class lacks an assetName
field which reduces observability compared to the existing DownloadAsset action;
update the DetailsAction sealed types by adding a val assetName: String
parameter to the OnDownloadForTransfer data class (mirroring DownloadAsset's
shape), update all call sites and ViewModel logging/analytics that construct or
consume OnDownloadForTransfer to pass and use assetName, and adjust any
serializers/tests that reference DetailsAction/OnDownloadForTransfer accordingly
so logs and error diagnostics include the asset name.
πͺ 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: dde7f492-d1cf-427b-bf22-17501135edcc
π Files selected for processing (4)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsAction.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ReleaseAssetsPicker.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Header.kt
π§ Files skipped from review as they are similar to previous changes (2)
- feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Header.kt
- feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.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.
Sprint 2 task E15. Survey #7 β users want to grab installers across OSes ("Android APKs from desktop, Linux .debs from phone").
What landed:
core/domain/util/AssetPlatform.ktβ filename βDiscoveryPlatformmapping (apk/exe/msi/dmg/pkg/deb/rpm/AppImage/snap/etc.).TweaksRepository.getShowAllPlatforms()/setShowAllPlatforms()β global persisted toggle, DataStore keyshow_all_platforms.DetailsState.showAllPlatformsobserved from preference.ReleaseAssetsItemsPicker(the sheet): Switch at top wired toOnToggleShowAllPlatforms. When ON, the list groups assets byDiscoveryPlatformwith section headers (Android / Windows / macOS / Linux). Non-installable-on-current-platform assets route toOnDownloadForTransferwhich opens the asset's downloadUrl viaBrowserHelper.openUrlβ browser handles save-to-Downloads. Hint row "Other platform β opens in browser to save for transfer" under each non-current asset.SelectDownloadAssetβ install button picks it up.Compile-verified:
:composeApp:compileDebugKotlinAndroidBUILD SUCCESSFUL.Summary by CodeRabbit
New Features
Localization