Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis release updates Rush to v6.3.3: add changelog and Android version metadata, replace raw millisecond delay args with Kotlin Duration, adopt PageFill(modifier) across pages and constrain scaffolds for wider screens, adjust lyrics and settings layouts for landscape/desktop, apply desktop density scaling, and bump dependency versions. Changesv6.3.3 Release: Layout Modernization and Desktop UX
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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. 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: 3
🧹 Nitpick comments (2)
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/component/PageFill.kt (1)
26-33: ⚡ Quick winUpdate KDoc to document the new
modifierparameter.The KDoc should include documentation for the newly added
modifierparameter to maintain API documentation completeness.📝 Suggested KDoc update
/** * A composable that fills the entire available screen space and centers its content. This is a * convenience wrapper around a [Box] with `Modifier.fillMaxSize()` and `contentAlignment = * Alignment.Center`. * + * `@param` modifier The [Modifier] to be applied to the Box. This modifier will be chained with + * [Modifier.fillMaxSize()], allowing callers to add constraints or styling. * `@param` content The composable content to be displayed in the center of the page. The content is * placed within a [BoxScope], allowing for more complex layouts if needed. */🤖 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 `@shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/component/PageFill.kt` around lines 26 - 33, The KDoc for the PageFill composable was not updated to document the new modifier parameter; update the KDoc above the PageFill function to add an `@param` entry for modifier that explains it accepts a Modifier applied to the outer Box (default Modifier), and keep the existing `@param` for content unchanged so the documentation lists both parameters (modifier and content) and their purposes.shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/saved/SavedPage.kt (1)
99-99: ⚡ Quick winExtract the
700.dpmax-width constraint to a shared constant.The
700.dpwidth constraint is duplicated across multiple pages. Extracting it to a named constant improves maintainability and makes the design intent clearer.
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/saved/SavedPage.kt#L99-L99: replacemax = 700.dpwith a shared constantshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/About.kt#L88-L88: replacemax = 700.dpwith a shared constantshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/BackupPage.kt#L130-L130: replacemax = 700.dpwith a shared constantshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/Changelog.kt#L64-L64: replacemax = 700.dpwith a shared constant♻️ Example: Define a shared constant
In a common UI constants file (e.g.,
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/Constants.kt):package com.shub39.rush.shared.ui import androidx.compose.ui.unit.dp /** Maximum width for page content on larger screens (landscape/desktop). */ val PageContentMaxWidth = 700.dpThen replace all instances:
-Modifier.widthIn(max = 700.dp) +Modifier.widthIn(max = PageContentMaxWidth)🤖 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 `@shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/saved/SavedPage.kt` at line 99, Create a shared UI constant and replace the duplicated 700.dp usages: add a new file shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/Constants.kt declaring a public val PageContentMaxWidth = 700.dp in package com.shub39.rush.shared.ui, then update each site to use that constant (replace `max = 700.dp` with `max = PageContentMaxWidth`): anchor shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/saved/SavedPage.kt (line 99) — change modifier = Modifier.widthIn(max = 700.dp) to use PageContentMaxWidth; sibling shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/About.kt (line 88) — replace max = 700.dp with PageContentMaxWidth; sibling shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/BackupPage.kt (line 130) — replace max = 700.dp with PageContentMaxWidth; sibling shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/Changelog.kt (line 64) — replace max = 700.dp with PageContentMaxWidth. Ensure you import com.shub39.rush.shared.ui.PageContentMaxWidth where needed.
🤖 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 `@desktopApp/src/commonMain/kotlin/com/shub39/rush/Main.kt`:
- Around line 46-51: The Density scope is incorrectly compounding text scaling
by multiplying both density and fontScale; update the scaledDensity construction
in Main.kt so that only currentDensity.density is multiplied by scale (keep
fontScale as currentDensity.fontScale unchanged) — i.e., set Density(density =
currentDensity.density * scale, fontScale = currentDensity.fontScale) for the
scaledDensity variable.
In
`@shared/core/src/jvmMain/kotlin/com/shub39/rush/shared/core/listener/MediaListener.jvm.kt`:
- Line 50: The change reduced the polling sleep to delay(500.milliseconds) which
doubles external playerctl command churn because updateMediaInfo() runs four
commands per loop; revert the sleep to the previous cadence (e.g.,
delay(1.seconds) or the original Duration value used before this change) while
keeping the new Duration type usage, so only the type was changed and the
polling frequency remains unchanged; update the delay call inside the loop that
calls updateMediaInfo() accordingly.
In
`@shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/lyrics/section/LyricsPage.kt`:
- Around line 263-270: The Column in LyricsPage.kt that currently uses
Modifier.fillMaxWidth().padding(start = 16.dp, end = 16.dp, top = 48.dp) should
be made inset-aware: remove the hardcoded top = 48.dp and instead use a
status-bar aware modifier like Modifier.statusBarsPadding() (or combine
WindowInsets.systemBars.asPaddingValues().calculateTopPadding()) so the header
adapts to device cutouts; update the Column's modifier to chain
statusBarsPadding() (e.g.,
Modifier.fillMaxWidth().statusBarsPadding().padding(start = 16.dp, end = 16.dp,
top = <optional small extra dp if needed>)) and ensure imports for
androidx.compose.foundation.layout.statusBarsPadding (or WindowInsets APIs) are
added.
---
Nitpick comments:
In
`@shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/component/PageFill.kt`:
- Around line 26-33: The KDoc for the PageFill composable was not updated to
document the new modifier parameter; update the KDoc above the PageFill function
to add an `@param` entry for modifier that explains it accepts a Modifier applied
to the outer Box (default Modifier), and keep the existing `@param` for content
unchanged so the documentation lists both parameters (modifier and content) and
their purposes.
In
`@shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/saved/SavedPage.kt`:
- Line 99: Create a shared UI constant and replace the duplicated 700.dp usages:
add a new file
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/Constants.kt declaring
a public val PageContentMaxWidth = 700.dp in package com.shub39.rush.shared.ui,
then update each site to use that constant (replace `max = 700.dp` with `max =
PageContentMaxWidth`): anchor
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/saved/SavedPage.kt
(line 99) — change modifier = Modifier.widthIn(max = 700.dp) to use
PageContentMaxWidth; sibling
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/About.kt
(line 88) — replace max = 700.dp with PageContentMaxWidth; sibling
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/BackupPage.kt
(line 130) — replace max = 700.dp with PageContentMaxWidth; sibling
shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/Changelog.kt
(line 64) — replace max = 700.dp with PageContentMaxWidth. Ensure you import
com.shub39.rush.shared.ui.PageContentMaxWidth where needed.
🪄 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: da8e0513-144b-41a7-ae2c-8b73afafa56e
📒 Files selected for processing (26)
CHANGELOG.mdandroidApp/build.gradle.ktsandroidLibs/visualizer-helper/src/main/java/io/gitlab/bpavuk/viz/RememberVisualizerState.ktdesktopApp/src/commonMain/kotlin/com/shub39/rush/Main.ktgradle/libs.versions.tomlshared/core/src/jvmMain/kotlin/com/shub39/rush/shared/core/listener/MediaListener.jvm.ktshared/logic/src/commonMain/composeResources/files/changelog.jsonshared/ui/build.gradle.ktsshared/ui/src/androidMain/kotlin/com/shub39/rush/shared/ui/setting/Util.android.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/component/PageFill.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/lyrics/LyricsGraph.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/lyrics/component/customisation/LyricsCustomisationPreview.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/lyrics/component/customisation/LyricsCustomisationSettings.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/lyrics/section/LyricsCustomisationPage.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/lyrics/section/LyricsPage.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/onboarding/Onboarding.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/onboarding/component/AnimatedAppIcon.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/saved/SavedPage.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/searchsheet/SearchSheet.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/About.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/BackupPage.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/Changelog.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/LookAndFeelPage.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/setting/section/SettingRootPage.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/share/component/cards/MessyCard.ktshared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/viewmodels/LyricsVM.kt
💤 Files with no reviewable changes (2)
- shared/ui/src/commonMain/kotlin/com/shub39/rush/shared/ui/lyrics/LyricsGraph.kt
- shared/ui/build.gradle.kts
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
New Features
Bug Fixes
Improvements