feat(e14): first-time coachmark on per-app release-channel chip#586
Merged
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 (2)
WalkthroughAdds a one-shot channel-chip coachmark: DataStore-backed persistence, Details presentation action/state and ViewModel orchestration, pulsing chip + popup UI, and localized strings plus updated release notes. ChangesChannel Chip Coachmark
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. Comment |
Greptile Summary
Confidence Score: 4/5Safe to merge after resolving the raw-pixel Popup offset, which breaks coachmark positioning on mdpi/xxxhdpi devices. One confirmed P1 (hard-coded IntOffset in raw pixels rather than dp-converted) caps the score at 4. The DataStore + ViewModel logic is sound, the one-shot semantic is correct, and localization coverage is complete. The two P2 items (infinite transition when inactive, focusable=false) are non-blocking but should be addressed. ReleaseChannel.kt — Popup offset (P1 density bug), rememberChipPulse (P2 wasted animation), focusable=false (P2 accessibility). Important Files Changed
Sequence DiagramsequenceDiagram
participant VM as DetailsViewModel
participant DS as TweaksRepository (DataStore)
participant UI as ReleaseChannel (Compose)
participant User
VM->>DS: getChannelChipCoachmarkShown().first()
DS-->>VM: false (never shown)
VM->>VM: "_state.first { !isLoading }"
Note over VM: guard: installedApp != null?
VM->>VM: "_state.update(isChannelChipCoachmarkPending = true)"
VM-->>UI: "state.isChannelChipCoachmarkPending = true"
UI->>UI: "rememberChipPulse(active=true) → 1.0f↔1.06f"
UI->>UI: show ChannelChipCoachmark Popup
alt User taps Got it
User->>UI: click TextButton
UI->>VM: OnAcknowledgeChannelChipCoachmark
VM->>VM: "_state.update(isChannelChipCoachmarkPending = false)"
VM->>DS: setChannelChipCoachmarkShown(true)
else User taps channel chip
User->>UI: click ChannelChip
UI->>VM: ToggleIncludeBetas
VM->>VM: acknowledgeChannelChipCoachmark()
VM->>DS: setChannelChipCoachmarkShown(true)
VM->>VM: toggleIncludeBetas()
else User presses Back
User->>UI: "back press (dismissOnBackPress=true)"
UI->>VM: OnAcknowledgeChannelChipCoachmark (via onDismissRequest)
VM->>DS: setChannelChipCoachmarkShown(true)
end
Note over DS: flag persisted — coachmark never re-fires
Reviews (2): Last reviewed commit: "Update core/presentation/src/commonMain/..." | Re-trigger Greptile |
![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: 1
🧹 Nitpick comments (2)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/ReleaseChannel.kt (2)
286-298: 💤 Low valueConsider stopping the animation when inactive.
The infinite transition runs continuously even when
active = false, animating from 1f to 1f. This creates unnecessary animation frames and recompositions. Consider wrapping the animation in a conditional or usingAnimatedVisibilityto avoid running the animation when the coachmark isn't pending.`@Composable` private fun rememberChipPulse(active: Boolean): Float { val infiniteTransition = rememberInfiniteTransition(label = "channel-chip-pulse") return if (active) { infiniteTransition.animateFloat( initialValue = 1f, targetValue = 1.06f, animationSpec = infiniteRepeatable( animation = tween(durationMillis = 1100), repeatMode = RepeatMode.Reverse, ), label = "channel-chip-pulse-scale", ).value } else { 1f } }🤖 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/sections/ReleaseChannel.kt` around lines 286 - 298, The rememberChipPulse function currently creates a rememberInfiniteTransition and animateFloat even when active is false; change rememberChipPulse to only start the infinite transition/animateFloat when active is true and return a constant 1f otherwise (e.g., use rememberInfiniteTransition + animateFloat and return its .value when active, else return 1f), referencing rememberChipPulse, rememberInfiniteTransition and animateFloat to locate the logic and avoid needless animations and recompositions.
300-360: ⚡ Quick winConsider using
PopupPositionProviderfor more robust positioning instead of hardcoded offsets.The popup uses a fixed y-offset of -220dp with
Alignment.TopStart, which mirrors the pattern inInspectApkButton.kt. However, the codebase already usesPopupPositionProviderfor dynamic positioning inHomeRoot.kt'sPlatformsPopup. For better robustness across screen sizes and layouts (especially on small devices or when the Details screen is scrolled), consider adopting the dynamic positioning approach.The
focusable=false,dismissOnClickOutside=false, anddismissOnBackPress=truesettings are consistent with the one-shot coachmark pattern and intentional—they work as designed.🤖 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/sections/ReleaseChannel.kt` around lines 300 - 360, ChannelChipCoachmark currently uses a hardcoded IntOffset (y = -220) which breaks on different screens; replace the static offset/alignment usage in ChannelChipCoachmark with a PopupPositionProvider implementation (following the dynamic approach used by PlatformsPopup in HomeRoot.kt) that computes the popup position relative to an anchor bounds or parent layout, then pass that provider via the Popup's positionProvider parameter (keep existing PopupProperties: focusable=false, dismissOnBackPress=true, dismissOnClickOutside=false and the same onDismissRequest). Locate ChannelChipCoachmark and implement a small PopupPositionProvider class/function that calculates safe x/y to avoid clipping (respecting screen/window insets) and use that provider in the Popup call instead of offset/alignment.
🤖 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/DetailsViewModel.kt`:
- Around line 558-567: The early return in acknowledgeChannelChipCoachmark
prevents persisting the coachmark acknowledgement when
_state.value.isChannelChipCoachmarkPending is false; change the function so that
persistence via viewModelScope.launch { runCatching {
tweaksRepository.setChannelChipCoachmarkShown(true) }... } always runs
(regardless of the pending flag) while keeping the existing _state.update {
it.copy(isChannelChipCoachmarkPending = false) } behavior idempotent; e.g.,
remove or move the early return and ensure
tweaksRepository.setChannelChipCoachmarkShown(true) is invoked (with the
existing onFailure logger) and the state update still happens via _state.update
in acknowledgeChannelChipCoachmark.
---
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/ReleaseChannel.kt`:
- Around line 286-298: The rememberChipPulse function currently creates a
rememberInfiniteTransition and animateFloat even when active is false; change
rememberChipPulse to only start the infinite transition/animateFloat when active
is true and return a constant 1f otherwise (e.g., use rememberInfiniteTransition
+ animateFloat and return its .value when active, else return 1f), referencing
rememberChipPulse, rememberInfiniteTransition and animateFloat to locate the
logic and avoid needless animations and recompositions.
- Around line 300-360: ChannelChipCoachmark currently uses a hardcoded IntOffset
(y = -220) which breaks on different screens; replace the static
offset/alignment usage in ChannelChipCoachmark with a PopupPositionProvider
implementation (following the dynamic approach used by PlatformsPopup in
HomeRoot.kt) that computes the popup position relative to an anchor bounds or
parent layout, then pass that provider via the Popup's positionProvider
parameter (keep existing PopupProperties: focusable=false,
dismissOnBackPress=true, dismissOnClickOutside=false and the same
onDismissRequest). Locate ChannelChipCoachmark and implement a small
PopupPositionProvider class/function that calculates safe x/y to avoid clipping
(respecting screen/window insets) and use that provider in the Popup call
instead of offset/alignment.
🪄 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: e9116a5e-2d0f-46ae-a229-baffa7014621
📒 Files selected for processing (20)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/TweaksRepository.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/sections/ReleaseChannel.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/presentation/src/commonMain/composeResources/values-hi/strings-hi.xml`:
- Around line 773-775: Update the coachmark dismiss text to be consistent with
other instructional coachmarks by changing the string resource
channel_chip_coachmark_dismiss from the current wording to the same
acknowledgment used elsewhere (e.g., "समझ गया"); locate and edit the string with
name="channel_chip_coachmark_dismiss" so it matches the APK inspect coachmark
dismiss string used elsewhere for consistent UX.
🪄 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
Run ID: c95b94e3-d0db-4eb1-bacf-bbdf662d789b
📒 Files selected for processing (13)
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.xml
✅ Files skipped from review due to trivial changes (4)
- core/presentation/src/commonMain/composeResources/values/strings.xml
- core/presentation/src/commonMain/composeResources/values-ko/strings-ko.xml
- core/presentation/src/commonMain/composeResources/values-ru/strings-ru.xml
- core/presentation/src/commonMain/composeResources/values-ja/strings-ja.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- core/presentation/src/commonMain/composeResources/values-it/strings-it.xml
…rings-hi.xml 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.
Per Sprint 2 brief E14. Survey #17 — users don't realise the per-app channel chip on Details toggles beta releases. Add discoverability nudge.
Scope (this PR):
channel_chip_coachmark_shownflag in TweaksRepository / DataStore.installedApp != nullso the chip is actually rendered when the pulse fires.DetailsAction.ToggleIncludeBetas) or explicitly viaOnAcknowledgeChannelChipCoachmark. Either path flips the persistent flag — coachmark never re-fires.Out of scope:
TweaksRepository.getIncludePreReleases(), applied to new tracks in InstallationManagerImpl) — only the discoverability half of E14 was missing.Compile-verified:
:composeApp:compileDebugKotlinAndroidBUILD SUCCESSFUL.Summary by CodeRabbit
New Features
Localization
Documentation