Conversation
Currently translated at 100.0% (152 of 152 strings) Translation: Rush/strings Translate-URL: https://hosted.weblate.org/projects/rush/strings/pl/
Currently translated at 100.0% (152 of 152 strings) Translation: Rush/strings Translate-URL: https://hosted.weblate.org/projects/rush/strings/pl/
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBumps release to v5.8.0 and SDK/tools, adopts Material Expressive theming and MotionScheme-based animation specs across Compose UI, refactors DotLoadingProgress to a lambda progress API, inserts placeholder TTML lines for large timing gaps, and updates Polish translations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 (6)
gradle/libs.versions.toml (1)
4-5: Keepvisualizer-helperon the cataloged SDK level.
visualizer-helper/build.gradle.kts:6-12still hard-codescompileSdk = 36, so this bump leaves that module behind the rest of the project. Please switch that module tolibs.versions.compileSdkas well so the repo keeps a single source of truth for SDK versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/libs.versions.toml` around lines 4 - 5, The visualizer-helper module is still hard-coding compileSdk = 36; update visualizer-helper/build.gradle.kts to use the cataloged SDK version (libs.versions.compileSdk or the DSL accessor used in the project) instead of the literal 36 and likewise align targetSdk if it’s hard-coded, replacing the compileSdk literal with the project’s libs.versions.compileSdk reference so the repo has a single source of truth.app/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.kt (1)
335-343: Prefer a localized label overPaletteStyle.toString().This subtitle is now built from the enum name, so values like
TONAL_SPOTwill render asTonal_spot. Please use a string resource/helper here, or at least normalize underscores before title-casing, so the label stays readable and translatable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.kt` around lines 335 - 343, The subtitle currently builds a label from the enum name (state.theme.style.toString()) which yields awkward, non-localized text like "Tonal_spot"; update LookAndFeelPage.kt to use a localized string or normalize the enum name before title-casing: either map PaletteStyle enum values to string resources (preferred) and fetch via stringResource, or at minimum convert the enum name with replace('_', ' ') then apply titlecase using LocalLocale.current.platformLocale so underscores are removed and the label is localizable/readable; reference the usage at state.theme.style in the supportingContent block to implement the change.app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt (2)
498-502: Same consideration for consistency withActions.kt.Similar to the previous comment, this uses
fastSpatialSpec()for fade transitions while other files usefastEffectsSpec().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt` around lines 498 - 502, The AnimatedVisibility block in LyricsPage.kt uses MaterialTheme.motionScheme.fastSpatialSpec() for fadeIn/fadeOut, causing inconsistency with other files (e.g., Actions.kt) that use fastEffectsSpec(); update the fadeIn and fadeOut calls inside the AnimatedVisibility (the block referencing state.sync and fadeIn/fadeOut) to use MaterialTheme.motionScheme.fastEffectsSpec() so transitions match the rest of the codebase.
429-434: Consider usingfastEffectsSpec()for fade transitions.This
AnimatedVisibilityusesfastSpatialSpec()forfadeIn/fadeOuttransitions, whileActions.ktusesfastEffectsSpec()for similar fade animations.
fastEffectsSpec()is typically intended for non-spatial effects (color, opacity), whereasfastSpatialSpec()is for spatial transformations (scale, position). For fade transitions,fastEffectsSpec()may be more semantically appropriate.If this is intentional (e.g., you want different timing characteristics here), please disregard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt` around lines 429 - 434, The fade transitions inside the AnimatedVisibility in LyricsPage (the block using visible = state.autoChange and the fadeIn/fadeOut calls) are using MaterialTheme.motionScheme.fastSpatialSpec(), but fade is a non-spatial effect; change those calls to MaterialTheme.motionScheme.fastEffectsSpec() for both fadeIn and fadeOut to match semantic intent and the usage in Actions.kt (unless the spatial timing was intentional).app/src/main/java/com/shub39/rush/presentation/lyrics/component/Actions.kt (1)
126-136: Missing motion spec for consistency.This
AnimatedVisibilityblock (for the edit/correct lyrics button) doesn't have explicitenter/exittransitions withMaterialTheme.motionScheme.fastEffectsSpec(), unlike all otherAnimatedVisibilityblocks in this file.♻️ Suggested fix for consistency
- AnimatedVisibility(visible = state.source == Sources.LRCLIB && state.selectedLines.isEmpty()) { + AnimatedVisibility( + visible = state.source == Sources.LRCLIB && state.selectedLines.isEmpty(), + enter = fadeIn(MaterialTheme.motionScheme.fastEffectsSpec()), + exit = fadeOut(MaterialTheme.motionScheme.fastEffectsSpec()) + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/shub39/rush/presentation/lyrics/component/Actions.kt` around lines 126 - 136, The AnimatedVisibility that shows the edit/correct button (AnimatedVisibility where visible = state.source == Sources.LRCLIB && state.selectedLines.isEmpty()) is missing explicit enter/exit motion specs; update that AnimatedVisibility to pass enter and exit transitions using MaterialTheme.motionScheme.fastEffectsSpec() (consistent with other blocks in this file) so its appearance/disappearance uses the same motion scheme; keep the existing onClick behavior invoking LyricsPageAction.OnLyricsCorrect(true), LyricsPageAction.OnSync(false), and possibly LyricsPageAction.OnToggleAutoChange.app/src/main/java/com/shub39/rush/presentation/components/ColorPickerDialog.kt (1)
74-78: Padding removal creates inconsistency with landscape layout.The portrait layout no longer has outer padding (Line 75), while the landscape layout at Line 115 retains
.padding(16.dp). This may cause visual inconsistency between orientations.Was this intentional? If so, consider documenting why portrait needs different padding behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/shub39/rush/presentation/components/ColorPickerDialog.kt` around lines 74 - 78, The portrait Column in ColorPickerDialog uses Modifier.wrapContentSize() with no padding, causing inconsistency with the landscape branch that uses .padding(16.dp); make the padding consistent by adding .padding(16.dp) to the portrait Column modifier (or alternatively remove .padding(16.dp) from the landscape branch or apply a single conditional modifier based on orientation inside the ColorPickerDialog composable) so both orientations render with the same outer spacing; if the omission was intentional, add a brief comment in ColorPickerDialog explaining why portrait omits padding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/shub39/rush/domain/dataclasses/TTMLParser.kt`:
- Around line 176-183: The injected blank-line filler is using line.startTime
when line.words is empty, which can hide an active lyric parsed via
getDirectTextContent(...); update the logic in TTMLParser.kt inside the
lines.forEachIndexed loop (where add(ParsedLine(text = "", startTime =
currentLineEnd, ...)) is created) to use the real line end timestamp parsed from
the <p> element (the parsed line's explicit end/dur value) as the fallback
instead of line.startTime, and if no explicit end/dur is available then skip
inserting the placeholder altogether; locate the code around the currentLineEnd
calculation (uses line.words.lastOrNull()?.endTime ?: line.startTime) and change
the fallback to use line.endTime or omit the filler when endTime is null.
In `@app/src/main/java/com/shub39/rush/presentation/components/RushTheme.kt`:
- Around line 49-50: The dynamic-color branch in RushTheme.kt skips Android 12
because the check uses > Build.VERSION_CODES.S; change the condition in the
theme.materialTheme guard to include API 31 by using >= Build.VERSION_CODES.S so
Android 12 devices hit the dynamic color path (the block that calls
colorResource(android.R.color.system_accent1_900)). Update the conditional where
theme.materialTheme is evaluated to use >= so LookAndFeelPage and the app theme
behavior align.
In
`@app/src/main/java/com/shub39/rush/presentation/lyrics/component/SyllableSyncedLyrics.kt`:
- Around line 339-344: The non-word line branch in SyllableSyncedLyrics (the
Text composable using isCurrent, textColor and underTextAlpha) immediately
applies underTextAlpha when not current, causing abrupt dimming; change the
color logic to treat already-completed lines the same as the word-timed path
(keep full textColor) and only apply underTextAlpha for truly future/unused
lines—e.g., compute or reuse an isCompleted/isPast flag (matching the word-timed
completion check) and use color = if (isCurrent || isCompleted) textColor else
textColor.copy(alpha = underTextAlpha).
---
Nitpick comments:
In
`@app/src/main/java/com/shub39/rush/presentation/components/ColorPickerDialog.kt`:
- Around line 74-78: The portrait Column in ColorPickerDialog uses
Modifier.wrapContentSize() with no padding, causing inconsistency with the
landscape branch that uses .padding(16.dp); make the padding consistent by
adding .padding(16.dp) to the portrait Column modifier (or alternatively remove
.padding(16.dp) from the landscape branch or apply a single conditional modifier
based on orientation inside the ColorPickerDialog composable) so both
orientations render with the same outer spacing; if the omission was
intentional, add a brief comment in ColorPickerDialog explaining why portrait
omits padding.
In `@app/src/main/java/com/shub39/rush/presentation/lyrics/component/Actions.kt`:
- Around line 126-136: The AnimatedVisibility that shows the edit/correct button
(AnimatedVisibility where visible = state.source == Sources.LRCLIB &&
state.selectedLines.isEmpty()) is missing explicit enter/exit motion specs;
update that AnimatedVisibility to pass enter and exit transitions using
MaterialTheme.motionScheme.fastEffectsSpec() (consistent with other blocks in
this file) so its appearance/disappearance uses the same motion scheme; keep the
existing onClick behavior invoking LyricsPageAction.OnLyricsCorrect(true),
LyricsPageAction.OnSync(false), and possibly
LyricsPageAction.OnToggleAutoChange.
In `@app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt`:
- Around line 498-502: The AnimatedVisibility block in LyricsPage.kt uses
MaterialTheme.motionScheme.fastSpatialSpec() for fadeIn/fadeOut, causing
inconsistency with other files (e.g., Actions.kt) that use fastEffectsSpec();
update the fadeIn and fadeOut calls inside the AnimatedVisibility (the block
referencing state.sync and fadeIn/fadeOut) to use
MaterialTheme.motionScheme.fastEffectsSpec() so transitions match the rest of
the codebase.
- Around line 429-434: The fade transitions inside the AnimatedVisibility in
LyricsPage (the block using visible = state.autoChange and the fadeIn/fadeOut
calls) are using MaterialTheme.motionScheme.fastSpatialSpec(), but fade is a
non-spatial effect; change those calls to
MaterialTheme.motionScheme.fastEffectsSpec() for both fadeIn and fadeOut to
match semantic intent and the usage in Actions.kt (unless the spatial timing was
intentional).
In
`@app/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.kt`:
- Around line 335-343: The subtitle currently builds a label from the enum name
(state.theme.style.toString()) which yields awkward, non-localized text like
"Tonal_spot"; update LookAndFeelPage.kt to use a localized string or normalize
the enum name before title-casing: either map PaletteStyle enum values to string
resources (preferred) and fetch via stringResource, or at minimum convert the
enum name with replace('_', ' ') then apply titlecase using
LocalLocale.current.platformLocale so underscores are removed and the label is
localizable/readable; reference the usage at state.theme.style in the
supportingContent block to implement the change.
In `@gradle/libs.versions.toml`:
- Around line 4-5: The visualizer-helper module is still hard-coding compileSdk
= 36; update visualizer-helper/build.gradle.kts to use the cataloged SDK version
(libs.versions.compileSdk or the DSL accessor used in the project) instead of
the literal 36 and likewise align targetSdk if it’s hard-coded, replacing the
compileSdk literal with the project’s libs.versions.compileSdk reference so the
repo has a single source of truth.
🪄 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: cf4e45f6-f885-4796-b6be-1e23ce126a52
📒 Files selected for processing (19)
CHANGELOG.mdapp/build.gradle.ktsapp/src/main/java/com/shub39/rush/domain/dataclasses/TTMLParser.ktapp/src/main/java/com/shub39/rush/presentation/components/ColorPickerDialog.ktapp/src/main/java/com/shub39/rush/presentation/components/RushTheme.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/Util.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/Actions.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/DotLoadingProgress.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/LineSyncedLyrics.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/PlainLyrics.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/SyllableSyncedLyrics.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/customisation/LyricsCustomisationSettings.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.ktapp/src/main/java/com/shub39/rush/presentation/setting/component/AboutApp.ktapp/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.ktapp/src/main/java/com/shub39/rush/presentation/setting/section/SettingRootPage.ktapp/src/main/java/com/shub39/rush/presentation/share/SharePage.ktapp/src/main/res/values-pl/strings.xmlgradle/libs.versions.toml
Comment on lines
+49
to
+50
| if (theme.materialTheme && Build.VERSION.SDK_INT > Build.VERSION_CODES.S) { | ||
| colorResource(android.R.color.system_accent1_900) |
There was a problem hiding this comment.
Include Android 12 in the dynamic-color path.
Line 49 uses > Build.VERSION_CODES.S, so API 31 falls through to Color(theme.seedColor). LookAndFeelPage already treats >= Build.VERSION_CODES.S as eligible for Material theming, which means Android 12 can show dynamic-color settings/previews while the actual app theme still ignores them.
Suggested fix
- if (theme.materialTheme && Build.VERSION.SDK_INT > Build.VERSION_CODES.S) {
+ if (theme.materialTheme && Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {📝 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
| if (theme.materialTheme && Build.VERSION.SDK_INT > Build.VERSION_CODES.S) { | |
| colorResource(android.R.color.system_accent1_900) | |
| if (theme.materialTheme && Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { | |
| colorResource(android.R.color.system_accent1_900) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/shub39/rush/presentation/components/RushTheme.kt`
around lines 49 - 50, The dynamic-color branch in RushTheme.kt skips Android 12
because the check uses > Build.VERSION_CODES.S; change the condition in the
theme.materialTheme guard to include API 31 by using >= Build.VERSION_CODES.S so
Android 12 devices hit the dynamic color path (the block that calls
colorResource(android.R.color.system_accent1_900)). Update the conditional where
theme.materialTheme is evaluated to use >= so LookAndFeelPage and the app theme
behavior align.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.kt`:
- Around line 335-344: The string transformation in the supportingContent Text
uses inconsistent locales (lowercase() default vs
titlecase(LocalLocale.current.platformLocale)); update the lowering call to
explicitly use the same locale (e.g., call
lowercase(LocalLocale.current.platformLocale)) so both lowercase and titlecase
use LocalLocale.current.platformLocale when transforming
state.theme.style.toString() in the supportingContent lambda of LookAndFeelPage.
🪄 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: b5dbeab0-0954-48a8-a7be-6ef7c5b7d278
📒 Files selected for processing (8)
app/src/main/java/com/shub39/rush/presentation/lyrics/Util.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/Actions.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/LineSyncedLyrics.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/PlainLyrics.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/customisation/LyricsCustomisationSettings.ktapp/src/main/java/com/shub39/rush/presentation/setting/component/AboutApp.ktapp/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.ktapp/src/main/java/com/shub39/rush/presentation/share/SharePage.kt
✅ Files skipped from review due to trivial changes (2)
- app/src/main/java/com/shub39/rush/presentation/share/SharePage.kt
- app/src/main/java/com/shub39/rush/presentation/lyrics/component/customisation/LyricsCustomisationSettings.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/shub39/rush/presentation/lyrics/component/Actions.kt
- app/src/main/java/com/shub39/rush/presentation/setting/component/AboutApp.kt
Merged
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
2 participants
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
Improvements
Localization
Documentation