Conversation
📝 WalkthroughWalkthroughRefactors HTTP client usage to internal lazy clients in API classes, removes centralized HttpClientFactory and Tokens, moves title/artist normalization to domain, introduces Google SansFlex typography and flexible top app bars, and bumps multiple build dependencies; adds tests and minor import/UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Repo as "Repository"
participant Lrc as "LrcLibApi"
participant Client as "HttpClient (internal)"
participant LrcSvc as "LRC External Service"
Note over Lrc: LrcLibApi lazily creates HttpClient (OkHttp + Ktor config)
Repo->>Lrc: request getLrcLyrics(title, artist, album?)
Lrc->>Client: build/request /api/search (strategy 1..n)
Client->>LrcSvc: HTTP GET /api/search?params
LrcSvc-->>Client: 200 OK + payload
Client-->>Lrc: parsed List<LrcGetDto>
Lrc-->>Repo: first non-empty result or null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/src/main/java/com/shub39/rush/data/backup/ExportImpl.kt (1)
32-32:⚠️ Potential issue | 🟡 MinorRemove unused
@OptIn(ExperimentalTime::class)annotation and import.This method does not use any
kotlin.timeAPIs, andExperimentalTimeis still required for certain parts of thekotlin.timepackage in Kotlin 2.3.10. The annotation and import at line 24 are unused and should be removed. (Note:ExperimentalTimeremains experimental in recent Kotlin versions for APIs likeInt.seconds,Long.minutes, etc., though some core measurement types likeClockare available.)🤖 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/data/backup/ExportImpl.kt` at line 32, Remove the unused ExperimentalTime opt-in: delete the `@OptIn`(ExperimentalTime::class) annotation and its import from ExportImpl (the ExportImpl class / method where `@OptIn` is declared) since no kotlin.time APIs are used; ensure no other usages of ExperimentalTime remain in that file and run a quick compile to confirm the unused import/annotation are gone.app/build.gradle.kts (1)
27-28:⚠️ Potential issue | 🟡 MinorVersion mismatch: PR title says "5.6.0" but code still has "5.5.2".
appVersionNameis"5.5.2"andappVersionCodeis5520, but the PR title indicates this should be version 5.6.0. If this release is intended as 5.6.0, these values need updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/build.gradle.kts` around lines 27 - 28, Update the version constants to match the PR title: change the value of appVersionName and appVersionCode so they represent 5.6.0 (update the string in appVersionName and the numeric appVersionCode accordingly); locate and modify the appVersionName and appVersionCode declarations in build.gradle.kts to reflect the new release version.app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt (1)
340-360:⚠️ Potential issue | 🟡 MinorInconsistent typography between portrait and landscape layouts.
The portrait layout (Lines 248, 258) was updated to use
headlineSmall/titleSmallfor the song title and artist, but the landscape layout here still uses the oldtitleLarge/bodyMediumstyles. This looks like an oversight — the same typography migration should be applied to both orientations for visual consistency.Proposed fix
Text( text = lyricsState.song.title, - style = MaterialTheme.typography.titleLarge, - fontWeight = FontWeight.SemiBold, + style = MaterialTheme.typography.headlineSmall, maxLines = 1, overflow = TextOverflow.Ellipsis, modifier = Modifier.padding(horizontal = 16.dp) .basicMarquee(), ) Text( text = lyricsState.song.artists, - style = MaterialTheme.typography.bodyMedium, + style = MaterialTheme.typography.titleSmall, fontWeight = FontWeight.Bold, maxLines = 1, overflow = TextOverflow.Ellipsis, modifier = Modifier.padding(horizontal = 16.dp) .basicMarquee(), )🤖 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 340 - 360, The landscape layout still uses outdated typography for the song title and artist: change the two Text composables that render lyricsState.song.title and lyricsState.song.artists in LyricsPage.kt to use the same migrated styles as portrait (swap MaterialTheme.typography.titleLarge -> MaterialTheme.typography.headlineSmall for the title and MaterialTheme.typography.bodyMedium -> MaterialTheme.typography.titleSmall for the artist), keeping the existing fontWeight, maxLines, overflow and Modifier.basicMarquee() unchanged to preserve behavior.app/src/main/java/com/shub39/rush/data/backup/RestoreImpl.kt (1)
52-53:⚠️ Potential issue | 🟡 MinorNull input stream silently maps to a misleading error.
If
contentResolver.openInputStreamreturnsnull,input?.copyTo(output)is a no-op — the temp file stays empty.file.readText()then returns"", triggering aSerializationExceptionthat surfaces asRestoreFailedException.OldSchema. The user sees "old schema" when the real problem is that the file could not be opened.🐛 Proposed fix — detect null input and fail fast with the correct error
- context.contentResolver.openInputStream(path.toUri()).use { input -> - file.outputStream().use { output -> input?.copyTo(output) } - } + val inputStream = context.contentResolver.openInputStream(path.toUri()) + ?: throw IllegalArgumentException("Could not open input stream for $path") + inputStream.use { input -> + file.outputStream().use { output -> input.copyTo(output) } + }🤖 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/data/backup/RestoreImpl.kt` around lines 52 - 53, The code silently proceeds when context.contentResolver.openInputStream(path.toUri()) returns null, leaving an empty temp file and causing a misleading SerializationException/RestoreFailedException.OldSchema; update the block around context.contentResolver.openInputStream(path.toUri()).use { input -> ... } to explicitly check for null before using the stream (i.e., if input == null) and fail fast by throwing a clear RestoreFailedException (or add a new variant) indicating the input stream could not be opened for the given URI, rather than continuing to write an empty file and later calling file.readText() which masks the real error.
♻️ Duplicate comments (1)
app/src/main/java/com/shub39/rush/data/network/GeniusApi.kt (1)
41-64: Same duplicatedHttpClientsetup — see comment onGeniusScraper.kt.🤖 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/data/network/GeniusApi.kt` around lines 41 - 64, The HttpClient initialization in GeniusApi (the lazy property client) duplicates the same setup used elsewhere (e.g., GeniusScraper); refactor this by extracting the shared configuration into a single factory/provider function (e.g., createSharedHttpClient or HttpClientProvider.getClient) that encapsulates ContentNegotiation, HttpTimeout, conditional Logging (BuildConfig.DEBUG) and defaultRequest contentType, then replace the GeniusApi.client lazy block with a call to that shared factory so both GeniusApi and GeniusScraper reuse the same HttpClient construction logic.
🧹 Nitpick comments (13)
app/src/main/java/com/shub39/rush/presentation/setting/section/SettingRootPage.kt (1)
92-98: Nit: variable namescrollBehaviourvs API parameterscrollBehavior.The variable uses British spelling (
Behaviour) while Compose APIs use American spelling (Behavior). Not a bug, but slightly inconsistent when reading the code.🤖 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/SettingRootPage.kt` around lines 92 - 98, Rename the British-spelled local variable scrollBehaviour to the API-consistent scrollBehavior: change the declaration using TopAppBarDefaults.enterAlwaysScrollBehavior() and update all usages (e.g., Modifier.nestedScroll(scrollBehaviour.nestedScrollConnection) and the LargeFlexibleTopAppBar(scrollBehavior = scrollBehaviour) to use scrollBehavior and scrollBehavior.nestedScrollConnection so names match the Compose API.README.md (1)
4-4: Nit: capitalize "GitHub" in the alt text.The alt text reads
"Get it on github"— the official name uses a capital "H".-[<img alt="Get it on github" src="badges/github.png" width="180px">](https://github.com/shub39/Rush/releases) +[<img alt="Get it on GitHub" src="badges/github.png" width="180px">](https://github.com/shub39/Rush/releases)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 4, Update the alt text in the Markdown image tag [<img alt="Get it on github" src="badges/github.png" width="180px">] to use the correct capitalization "Get it on GitHub" so the alt attribute reads alt="Get it on GitHub".CHANGELOG.md (1)
4-4: Nit: capitalize "api" → "API" for consistency."LRCLIB" is already uppercase; "api" looks inconsistent next to it.
-- Fixed LRCLIB api not responding sometimes +- Fixed LRCLIB API not responding sometimes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 4, Update the changelog entry string "LRCLIB api not responding sometimes" to use uppercase API for consistency by changing it to "LRCLIB API not responding sometimes"; modify the exact text in CHANGELOG.md where that line appears so "api" is capitalized to "API".app/src/main/java/com/shub39/rush/presentation/saved/SavedPage.kt (1)
103-121: Consider wiringscrollBehaviorfor consistency with other pages.Other pages in this PR (e.g.,
SettingRootPage) wireTopAppBarDefaults.enterAlwaysScrollBehavior()toLargeFlexibleTopAppBaralong with anestedScrollmodifier on theScaffold. Here, no scroll behavior is connected, so the large top bar won't collapse when the list scrolls. If this is intentional (e.g., because of the sort-button row below), no action needed — but worth confirming.🤖 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/saved/SavedPage.kt` around lines 103 - 121, The LargeFlexibleTopAppBar is not wired to a scrollBehavior so it won't collapse with list scrolling; create a val scrollBehavior = TopAppBarDefaults.enterAlwaysScrollBehavior(), pass scrollBehavior = scrollBehavior into LargeFlexibleTopAppBar and add Modifier.nestedScroll(scrollBehavior.nestedScrollConnection) to the surrounding Scaffold (or top-level container) so the bar collapses consistently like SettingRootPage.app/src/main/java/com/shub39/rush/data/network/GeniusScraper.kt (2)
46-68: DuplicatedHttpClientconfiguration across network classes.This exact client setup (OkHttp engine, ContentNegotiation, HttpTimeout, conditional Logging, defaultRequest) is copy-pasted in
GeniusApi,GeniusScraper, and (per the summary)LrcLibApi. While the old centralizedHttpClientFactorywas removed intentionally, consider extracting a shared factory function (e.g.,internal fun createDefaultClient(): HttpClient) to avoid three copies drifting out of sync.Also,
connectTimeoutMillisis not set — onlysocketTimeoutMillisandrequestTimeoutMillis. If one of the scraped endpoints is slow to establish a TCP connection, there's no bound on that wait.🤖 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/data/network/GeniusScraper.kt` around lines 46 - 68, Extract the duplicated HttpClient construction used in GeniusScraper, GeniusApi and LrcLibApi into a single internal factory function (e.g., internal fun createDefaultClient(): HttpClient) and replace the lazy client properties in those classes to call this factory so configuration (OkHttp engine, ContentNegotiation, HttpTimeout, conditional Logging, defaultRequest) is centralized; also add connectTimeoutMillis to the HttpTimeout configuration so connect, socket and request timeouts are all bounded.
66-66:Content-Type: application/jsondefault on a scraper that only sends GET requests to HTML pages.This header is meaningless on bodiless GET requests and won't cause issues, but it's misleading. If you extract a shared client builder (per the above comment), you could omit the
defaultRequestblock here and only set it inGeniusApiwhere JSON requests are actually made.🤖 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/data/network/GeniusScraper.kt` at line 66, Remove the misleading defaultRequest { contentType(ContentType.Application.Json) } from the GeniusScraper Ktor client configuration (locate it in GeniusScraper.kt where the client is built) since the scraper only issues GET requests to HTML pages; instead, ensure the JSON Content-Type is applied only where JSON requests are made (e.g., in the GeniusApi client or request builders such as the class/method that builds API calls for GeniusApi). If you refactor to a shared client builder, keep the JSON default in the GeniusApi-specific client configuration or set the header per-request in the GeniusApi methods rather than globally in GeniusScraper.app/src/main/java/com/shub39/rush/presentation/setting/section/BackupPage.kt (1)
125-140: Minor naming inconsistency:scrollBehaviourvsscrollBehavior.The local variable uses British spelling (
scrollBehaviour) while the API parameter uses American spelling (scrollBehavior). Consider aligning toscrollBehaviorfor consistency with the Compose API naming convention. Same applies inLookAndFeelPage.kt.🤖 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/BackupPage.kt` around lines 125 - 140, Rename the local variable scrollBehaviour to scrollBehavior to match the Compose API naming and avoid inconsistency: change the variable created from TopAppBarDefaults.enterAlwaysScrollBehavior() and all its usages (e.g., nestedScroll(scrollBehaviour.nestedScrollConnection) and scrollBehavior = scrollBehaviour passed into MediumFlexibleTopAppBar) to use scrollBehavior instead; apply the same rename in LookAndFeelPage.kt so both files consistently use scrollBehavior and update any references to nestedScrollConnection accordingly.app/src/main/java/com/shub39/rush/presentation/provideTypography.kt (1)
35-69: Flex font families are recreated on every call — consider hoisting them.
flexFontDisplay,flexFontHeadline, andflexFontTitledon't depend on any parameters or composition state. Defining them as top-levelvals (alongsideTYPOGRAPHY) avoids allocating newFontFamily/Fontobjects on each recomposition.Proposed refactor
+@OptIn(ExperimentalTextApi::class) +private val flexFontDisplay = + FontFamily( + Font( + resId = R.font.google_sans_flex, + variationSettings = + FontVariation.Settings( + FontVariation.weight(900), + FontVariation.slant(-6f), + FontVariation.width(120f), + ), + ) + ) + +@OptIn(ExperimentalTextApi::class) +private val flexFontHeadline = + FontFamily( + Font( + resId = R.font.google_sans_flex, + variationSettings = + FontVariation.Settings( + FontVariation.weight(800), + FontVariation.slant(-6f), + FontVariation.width(110f), + ), + ) + ) + +@OptIn(ExperimentalTextApi::class) +private val flexFontTitle = + FontFamily( + Font( + resId = R.font.google_sans_flex, + variationSettings = + FontVariation.Settings( + FontVariation.weight(500), + FontVariation.Setting("ROND", 100f), + ), + ) + ) + -@OptIn(ExperimentalTextApi::class) `@Composable` fun provideTypography(font: Int? = R.font.poppins_regular): Typography { - val flexFontDisplay = ... - val flexFontHeadline = ... - val flexFontTitle = ... val selectedFont = font?.let { FontFamily(Font(it)) } ?: FontFamily.Default ... }🤖 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/provideTypography.kt` around lines 35 - 69, The three FontFamily vals flexFontDisplay, flexFontHeadline, and flexFontTitle are recreated on every call; hoist them to top-level immutable vals so they are allocated once. Move the current declarations for flexFontDisplay, flexFontHeadline, and flexFontTitle out of the function/scope where they currently live and place them as top-level val declarations (alongside TYPOGRAPHY), keeping their Font(...) and FontVariation.Settings(...) definitions identical, then have the provideTypography code reference these top-level vals instead of local ones.app/src/main/java/com/shub39/rush/presentation/util.kt (1)
121-134:" X "entry is redundant —containson line 139 already usesignoreCase = true.Since
getMainArtistcallscontains(separator, ignoreCase = true)andsplit(..., ignoreCase = true, ...), the" x "entry already covers" X ". The explicit" X "on line 127 is dead code.Proposed fix
listOf( " & ", " and ", ", ", " x ", - " X ", " feat. ", " feat ", " ft. ", " ft ", " featuring ", " with ", )🤖 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/util.kt` around lines 121 - 134, The artistSeparators list contains a redundant " X " entry because getMainArtist calls contains(separator, ignoreCase = true) and split(..., ignoreCase = true), so the lowercase " x " already matches; remove the " X " element from the artistSeparators list to eliminate dead code and keep only the case-insensitive separators (update the list in the artistSeparators declaration and verify getMainArtist still uses contains(..., ignoreCase = true) and split(..., ignoreCase = true) as intended).app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt (2)
92-143:safeCallis used only for its exception-catching side-effect; success paths bypass itsHttpResponse→Resultconversion entirely.All success paths use non-local returns from the inline
safeCalllambda, sosafeCall'sresponseToResultlogic is never reached. This works becausesafeCallisinline, but it's fragile and misleading — the type parameterTandHttpResponsereturn type of the lambda are effectively unused. Consider extracting a simplertry/catch-based wrapper (e.g.,safeCatch) that only handles exceptions, or restructuring to not rely on non-local returns.🤖 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/data/network/LrcLibApi.kt` around lines 92 - 143, The method searchLrcLyrics currently wraps its whole body in the inline safeCall lambda but uses non-local returns on success so safeCall's HttpResponse→Result conversion (responseToResult) is never exercised; replace the use of safeCall with a simple try/catch that only handles exceptions (or introduce a new helper safeCatch) so searchLrcLyrics returns Result.Success/Result.Error directly from its success paths and exceptions are mapped to Result.Error in one place; update references to safeCall, responseToResult and keep searchLrcLyrics, queryLyricsWithParams and getMainTitle/getMainArtist intact so callers and behavior remain the same while removing reliance on non-local returns.
100-107: Repeated filter predicate across all strategies.The predicate
{ it.syncedLyrics != null || it.plainLyrics != null }is duplicated five times. Extract it into a named function or val for clarity and DRY.Proposed fix
Add a local helper at the top of the
safeCallblock:val hasLyrics: (LrcGetDto) -> Boolean = { it.syncedLyrics != null || it.plainLyrics != null }Then replace all
.filter { it.syncedLyrics != null || it.plainLyrics != null }with.filter(hasLyrics).🤖 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/data/network/LrcLibApi.kt` around lines 100 - 107, Extract the repeated predicate { it.syncedLyrics != null || it.plainLyrics != null } into a single named val or function inside the safeCall block (e.g. val hasLyrics: (LrcGetDto) -> Boolean = { it.syncedLyrics != null || it.plainLyrics != null }) and replace every .filter { it.syncedLyrics != null || it.plainLyrics != null } in the search strategies that call queryLyricsWithParams(...) with .filter(hasLyrics) so all five strategy results use the shared predicate; keep the predicate local to the safeCall scope and ensure the type matches the LrcGetDto returned by queryLyricsWithParams.app/src/test/java/LrcLibApiTest.kt (2)
26-30:testInhelper is duplicated across three test files.The identical
testInhelper exists inGeniusApiTest.kt(lines 25-29) andScraperTest.kt(lines 25-29). Consider extracting it into a shared test utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/LrcLibApiTest.kt` around lines 26 - 30, Extract the duplicated suspend-test helper into a single shared test utility by moving the top-level function testIn(title: String, block: suspend CoroutineScope.() -> Unit) = runBlocking { ... } out of LrcLibApiTest, GeniusApiTest and ScraperTest and into a new test helper file (e.g., TestUtils.kt) in the test sources; keep the exact signature and runBlocking usage so behavior doesn't change, remove the duplicate definitions from the three test files, and update those tests to import and call the shared testIn function instead of their local copies.
32-39: Test lacks assertions and relies on a live API.This test hits the real LRCLIB API, making it flaky in CI if the network is unavailable or the API is down. Additionally,
println(result.data)on success doesn't verify any property of the response — the test passes as long as the API returns something.Consider:
- Adding actual assertions on the result (e.g., non-empty list, expected fields populated).
- Marking this as an integration test (e.g., using JUnit categories or a naming convention) so it can be excluded from fast CI runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/LrcLibApiTest.kt` around lines 32 - 39, The testSearch function currently calls the real lrcLibApi.searchLrcLyrics and only prints result.data, which makes it flaky and assertion-less; update the test to either (A) mock lrcLibApi.searchLrcLyrics (or inject a test double) and assert on the returned Result.Success payload (e.g., non-empty list and required fields populated) instead of println, or (B) mark the test as an integration test (e.g., add a JUnit `@Tag`("integration") or rename to include “IntegrationTest”) and replace println with concrete assertions that verify Result.Success and expected properties; reference the testSearch method, the lrcLibApi.searchLrcLyrics call, and the Result.Error/Result.Success branches when making the change.
🤖 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/data/backup/RestoreImpl.kt`:
- Around line 28-31: The RestoreImpl.kt code uses kotlin.io.path APIs
(createTempFile, outputStream, readText, deleteIfExists) which require Android
API 26; fix by either raising the app minSdk to 26 (update the project config)
OR enable core-library desugaring: set coreLibraryDesugaring true in the android
block and add the desugar_jdk_libs dependency
(com.android.tools:desugar_jdk_libs) so those java.nio APIs are available on
older devices; alternatively, replace the kotlin.io.path calls in RestoreImpl
(createTempFile, outputStream, readText, deleteIfExists) with equivalent
java.io/File APIs (File.createTempFile, File.outputStream/writeBytes,
File.readText, File.delete) to maintain minSdk 24 without desugaring.
In `@app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt`:
- Around line 135-142: The Strategy 5 branch in LrcLibApi.kt currently assigns
results from queryLyricsWithParams to the local variable results but never
returns them, so the function always falls through to return
Result.Error(SourceError.Data.NO_RESULTS); update the Strategy 5 block in the
method that calls queryLyricsWithParams (referencing the variable results and
the queryLyricsWithParams(trackName, artistName) call) to check if the filtered
results list is non-empty and immediately return those results (e.g.,
wrap/return as the same successful Result type the method uses) instead of
letting execution continue to the final NO_RESULTS error return.
- Around line 24-25: Move the pure string-cleaning functions getMainTitle and
getMainArtist out of the presentation layer and into the domain module, then
update all imports in consumers (notably LrcLibApi and MediaListenerImpl) to
reference the new domain package; specifically, relocate the functions from
com.shub39.rush.presentation (presentation.util.kt) into an appropriate domain
package (e.g., com.shub39.rush.domain.util), export them from the domain module,
and change import statements in LrcLibApi and MediaListenerImpl to use the new
domain package so the data layer no longer depends on presentation.
In `@app/src/main/java/com/shub39/rush/presentation/util.kt`:
- Around line 136-145: getMainArtist currently stops at the first found
separator which can leave secondary separators in the result (e.g., "Artist1
feat. Artist2 & Artist3" -> "Artist1 feat. Artist2"); update getMainArtist to
either (preferred) iterate through all entries in artistSeparators (or
repeatedly split until no separator remains) and always take the substring
before the earliest/first-occurring separator, or else reorder artistSeparators
to put "feat." variants before " & " so priority is correct; specifically modify
getMainArtist to scan all separators (or loop until no change) using
artistSeparators and return the fully cleaned main artist.
---
Outside diff comments:
In `@app/build.gradle.kts`:
- Around line 27-28: Update the version constants to match the PR title: change
the value of appVersionName and appVersionCode so they represent 5.6.0 (update
the string in appVersionName and the numeric appVersionCode accordingly); locate
and modify the appVersionName and appVersionCode declarations in
build.gradle.kts to reflect the new release version.
In `@app/src/main/java/com/shub39/rush/data/backup/ExportImpl.kt`:
- Line 32: Remove the unused ExperimentalTime opt-in: delete the
`@OptIn`(ExperimentalTime::class) annotation and its import from ExportImpl (the
ExportImpl class / method where `@OptIn` is declared) since no kotlin.time APIs
are used; ensure no other usages of ExperimentalTime remain in that file and run
a quick compile to confirm the unused import/annotation are gone.
In `@app/src/main/java/com/shub39/rush/data/backup/RestoreImpl.kt`:
- Around line 52-53: The code silently proceeds when
context.contentResolver.openInputStream(path.toUri()) returns null, leaving an
empty temp file and causing a misleading
SerializationException/RestoreFailedException.OldSchema; update the block around
context.contentResolver.openInputStream(path.toUri()).use { input -> ... } to
explicitly check for null before using the stream (i.e., if input == null) and
fail fast by throwing a clear RestoreFailedException (or add a new variant)
indicating the input stream could not be opened for the given URI, rather than
continuing to write an empty file and later calling file.readText() which masks
the real error.
In `@app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt`:
- Around line 340-360: The landscape layout still uses outdated typography for
the song title and artist: change the two Text composables that render
lyricsState.song.title and lyricsState.song.artists in LyricsPage.kt to use the
same migrated styles as portrait (swap MaterialTheme.typography.titleLarge ->
MaterialTheme.typography.headlineSmall for the title and
MaterialTheme.typography.bodyMedium -> MaterialTheme.typography.titleSmall for
the artist), keeping the existing fontWeight, maxLines, overflow and
Modifier.basicMarquee() unchanged to preserve behavior.
---
Duplicate comments:
In `@app/src/main/java/com/shub39/rush/data/network/GeniusApi.kt`:
- Around line 41-64: The HttpClient initialization in GeniusApi (the lazy
property client) duplicates the same setup used elsewhere (e.g., GeniusScraper);
refactor this by extracting the shared configuration into a single
factory/provider function (e.g., createSharedHttpClient or
HttpClientProvider.getClient) that encapsulates ContentNegotiation, HttpTimeout,
conditional Logging (BuildConfig.DEBUG) and defaultRequest contentType, then
replace the GeniusApi.client lazy block with a call to that shared factory so
both GeniusApi and GeniusScraper reuse the same HttpClient construction logic.
---
Nitpick comments:
In `@app/src/main/java/com/shub39/rush/data/network/GeniusScraper.kt`:
- Around line 46-68: Extract the duplicated HttpClient construction used in
GeniusScraper, GeniusApi and LrcLibApi into a single internal factory function
(e.g., internal fun createDefaultClient(): HttpClient) and replace the lazy
client properties in those classes to call this factory so configuration (OkHttp
engine, ContentNegotiation, HttpTimeout, conditional Logging, defaultRequest) is
centralized; also add connectTimeoutMillis to the HttpTimeout configuration so
connect, socket and request timeouts are all bounded.
- Line 66: Remove the misleading defaultRequest {
contentType(ContentType.Application.Json) } from the GeniusScraper Ktor client
configuration (locate it in GeniusScraper.kt where the client is built) since
the scraper only issues GET requests to HTML pages; instead, ensure the JSON
Content-Type is applied only where JSON requests are made (e.g., in the
GeniusApi client or request builders such as the class/method that builds API
calls for GeniusApi). If you refactor to a shared client builder, keep the JSON
default in the GeniusApi-specific client configuration or set the header
per-request in the GeniusApi methods rather than globally in GeniusScraper.
In `@app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt`:
- Around line 92-143: The method searchLrcLyrics currently wraps its whole body
in the inline safeCall lambda but uses non-local returns on success so
safeCall's HttpResponse→Result conversion (responseToResult) is never exercised;
replace the use of safeCall with a simple try/catch that only handles exceptions
(or introduce a new helper safeCatch) so searchLrcLyrics returns
Result.Success/Result.Error directly from its success paths and exceptions are
mapped to Result.Error in one place; update references to safeCall,
responseToResult and keep searchLrcLyrics, queryLyricsWithParams and
getMainTitle/getMainArtist intact so callers and behavior remain the same while
removing reliance on non-local returns.
- Around line 100-107: Extract the repeated predicate { it.syncedLyrics != null
|| it.plainLyrics != null } into a single named val or function inside the
safeCall block (e.g. val hasLyrics: (LrcGetDto) -> Boolean = { it.syncedLyrics
!= null || it.plainLyrics != null }) and replace every .filter { it.syncedLyrics
!= null || it.plainLyrics != null } in the search strategies that call
queryLyricsWithParams(...) with .filter(hasLyrics) so all five strategy results
use the shared predicate; keep the predicate local to the safeCall scope and
ensure the type matches the LrcGetDto returned by queryLyricsWithParams.
In `@app/src/main/java/com/shub39/rush/presentation/provideTypography.kt`:
- Around line 35-69: The three FontFamily vals flexFontDisplay,
flexFontHeadline, and flexFontTitle are recreated on every call; hoist them to
top-level immutable vals so they are allocated once. Move the current
declarations for flexFontDisplay, flexFontHeadline, and flexFontTitle out of the
function/scope where they currently live and place them as top-level val
declarations (alongside TYPOGRAPHY), keeping their Font(...) and
FontVariation.Settings(...) definitions identical, then have the
provideTypography code reference these top-level vals instead of local ones.
In `@app/src/main/java/com/shub39/rush/presentation/saved/SavedPage.kt`:
- Around line 103-121: The LargeFlexibleTopAppBar is not wired to a
scrollBehavior so it won't collapse with list scrolling; create a val
scrollBehavior = TopAppBarDefaults.enterAlwaysScrollBehavior(), pass
scrollBehavior = scrollBehavior into LargeFlexibleTopAppBar and add
Modifier.nestedScroll(scrollBehavior.nestedScrollConnection) to the surrounding
Scaffold (or top-level container) so the bar collapses consistently like
SettingRootPage.
In
`@app/src/main/java/com/shub39/rush/presentation/setting/section/BackupPage.kt`:
- Around line 125-140: Rename the local variable scrollBehaviour to
scrollBehavior to match the Compose API naming and avoid inconsistency: change
the variable created from TopAppBarDefaults.enterAlwaysScrollBehavior() and all
its usages (e.g., nestedScroll(scrollBehaviour.nestedScrollConnection) and
scrollBehavior = scrollBehaviour passed into MediumFlexibleTopAppBar) to use
scrollBehavior instead; apply the same rename in LookAndFeelPage.kt so both
files consistently use scrollBehavior and update any references to
nestedScrollConnection accordingly.
In
`@app/src/main/java/com/shub39/rush/presentation/setting/section/SettingRootPage.kt`:
- Around line 92-98: Rename the British-spelled local variable scrollBehaviour
to the API-consistent scrollBehavior: change the declaration using
TopAppBarDefaults.enterAlwaysScrollBehavior() and update all usages (e.g.,
Modifier.nestedScroll(scrollBehaviour.nestedScrollConnection) and the
LargeFlexibleTopAppBar(scrollBehavior = scrollBehaviour) to use scrollBehavior
and scrollBehavior.nestedScrollConnection so names match the Compose API.
In `@app/src/main/java/com/shub39/rush/presentation/util.kt`:
- Around line 121-134: The artistSeparators list contains a redundant " X "
entry because getMainArtist calls contains(separator, ignoreCase = true) and
split(..., ignoreCase = true), so the lowercase " x " already matches; remove
the " X " element from the artistSeparators list to eliminate dead code and keep
only the case-insensitive separators (update the list in the artistSeparators
declaration and verify getMainArtist still uses contains(..., ignoreCase = true)
and split(..., ignoreCase = true) as intended).
In `@app/src/test/java/LrcLibApiTest.kt`:
- Around line 26-30: Extract the duplicated suspend-test helper into a single
shared test utility by moving the top-level function testIn(title: String,
block: suspend CoroutineScope.() -> Unit) = runBlocking { ... } out of
LrcLibApiTest, GeniusApiTest and ScraperTest and into a new test helper file
(e.g., TestUtils.kt) in the test sources; keep the exact signature and
runBlocking usage so behavior doesn't change, remove the duplicate definitions
from the three test files, and update those tests to import and call the shared
testIn function instead of their local copies.
- Around line 32-39: The testSearch function currently calls the real
lrcLibApi.searchLrcLyrics and only prints result.data, which makes it flaky and
assertion-less; update the test to either (A) mock lrcLibApi.searchLrcLyrics (or
inject a test double) and assert on the returned Result.Success payload (e.g.,
non-empty list and required fields populated) instead of println, or (B) mark
the test as an integration test (e.g., add a JUnit `@Tag`("integration") or rename
to include “IntegrationTest”) and replace println with concrete assertions that
verify Result.Success and expected properties; reference the testSearch method,
the lrcLibApi.searchLrcLyrics call, and the Result.Error/Result.Success branches
when making the change.
In `@CHANGELOG.md`:
- Line 4: Update the changelog entry string "LRCLIB api not responding
sometimes" to use uppercase API for consistency by changing it to "LRCLIB API
not responding sometimes"; modify the exact text in CHANGELOG.md where that line
appears so "api" is capitalized to "API".
In `@README.md`:
- Line 4: Update the alt text in the Markdown image tag [<img alt="Get it on
github" src="badges/github.png" width="180px">] to use the correct
capitalization "Get it on GitHub" so the alt attribute reads alt="Get it on
GitHub".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/src/main/res/font/google_sans.ttfis excluded by!**/*.ttfapp/src/main/res/font/google_sans_flex.ttfis excluded by!**/*.ttf
📒 Files selected for processing (30)
CHANGELOG.mdREADME.mdapp/build.gradle.ktsapp/src/main/java/com/shub39/rush/data/HttpClientFactory.ktapp/src/main/java/com/shub39/rush/data/backup/ExportImpl.ktapp/src/main/java/com/shub39/rush/data/backup/RestoreImpl.ktapp/src/main/java/com/shub39/rush/data/network/GeniusApi.ktapp/src/main/java/com/shub39/rush/data/network/GeniusScraper.ktapp/src/main/java/com/shub39/rush/data/network/LrcLibApi.ktapp/src/main/java/com/shub39/rush/data/network/Tokens.ktapp/src/main/java/com/shub39/rush/data/repository/RushRepository.ktapp/src/main/java/com/shub39/rush/di/RushModules.ktapp/src/main/java/com/shub39/rush/presentation/enumExt.ktapp/src/main/java/com/shub39/rush/presentation/glow.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/CurveVisualizer.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/SyncedLyrics.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.ktapp/src/main/java/com/shub39/rush/presentation/provideTypography.ktapp/src/main/java/com/shub39/rush/presentation/saved/SavedPage.ktapp/src/main/java/com/shub39/rush/presentation/setting/component/AboutApp.ktapp/src/main/java/com/shub39/rush/presentation/setting/section/BackupPage.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/java/com/shub39/rush/presentation/util.ktapp/src/test/java/GeniusApiTest.ktapp/src/test/java/LrcLibApiTest.ktapp/src/test/java/ScraperTest.ktgradle/libs.versions.tomlvisualizer-helper/build.gradle.kts
💤 Files with no reviewable changes (4)
- app/src/main/java/com/shub39/rush/data/network/Tokens.kt
- app/src/main/java/com/shub39/rush/di/RushModules.kt
- visualizer-helper/build.gradle.kts
- app/src/main/java/com/shub39/rush/data/HttpClientFactory.kt
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/src/main/java/com/shub39/rush/presentation/share/SharePage.kt (1)
154-156:⚠️ Potential issue | 🟠 MajorResource leak:
FileOutputStreammust be closed in ause {}block.If
imageBitmap.compress(...)throws (e.g. disk full, permission denied, OOM),stream.close()on line 156 is never reached, leaking the file descriptor. Use Kotlin'sCloseable.use {}to guarantee closure.🛡️ Proposed fix
-val stream = FileOutputStream(file) -imageBitmap.compress(Bitmap.CompressFormat.PNG, 100, stream) -stream.close() +FileOutputStream(file).use { stream -> + imageBitmap.compress(Bitmap.CompressFormat.PNG, 100, stream) +}🤖 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/share/SharePage.kt` around lines 154 - 156, Wrap the FileOutputStream creation and usage in a Kotlin use { } block to guarantee closure even if imageBitmap.compress(...) throws; replace the manual stream creation and stream.close() around the calls to FileOutputStream(...) / imageBitmap.compress(Bitmap.CompressFormat.PNG, 100, stream) in SharePage.kt with FileOutputStream(file).use { stream -> ... } so the stream is always closed; ensure any exceptions propagate or are handled as before.app/src/main/java/com/shub39/rush/presentation/saved/SavedPage.kt (1)
98-121:⚠️ Potential issue | 🟠 MajorMissing
scrollBehaviorwiring forLargeFlexibleTopAppBar.Every other page in this PR that uses a flexible top bar (
SettingRootPage,LookAndFeelPage,BackupPage) wires aTopAppBarDefaults.enterAlwaysScrollBehavior()to the bar and applies.nestedScroll(scrollBehaviour.nestedScrollConnection)on theScaffoldmodifier. This page does not, so the large top bar will remain fully expanded and never collapse on scroll, consuming significant vertical space.Proposed fix
Scaffold( - modifier = modifier, + modifier = modifier.nestedScroll(scrollBehaviour.nestedScrollConnection), topBar = { Column { if (!isLandscape) { LargeFlexibleTopAppBar( + scrollBehavior = scrollBehaviour, title = { Text(stringResource(R.string.rush_branding)) },And add before the
Scaffold:val scrollBehaviour = TopAppBarDefaults.enterAlwaysScrollBehavior()🤖 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/saved/SavedPage.kt` around lines 98 - 121, The LargeFlexibleTopAppBar is missing a scrollBehavior hookup so it won't collapse; define val scrollBehaviour = TopAppBarDefaults.enterAlwaysScrollBehavior() just before the Scaffold, pass scrollBehavior = scrollBehaviour into the LargeFlexibleTopAppBar call, and apply .nestedScroll(scrollBehaviour.nestedScrollConnection) to the Scaffold's modifier (i.e., use modifier.nestedScroll(scrollBehaviour.nestedScrollConnection)) so the app bar collapses on scroll.app/src/main/java/com/shub39/rush/data/backup/ExportImpl.kt (1)
39-43:⚠️ Potential issue | 🟠 Major
Log.wtfis inappropriate in a recoverablecatchblock and can terminate the process
Log.wtfsignals an unrecoverable programmer error ("What a Terrible Failure"). On Android, the defaultwtfhandler can kill the process (or trigger a crash report), which directly contradicts the intent of thistry/catchthat recovers by returningnull. UseLog.einstead.🐛 Proposed fix
- Log.wtf("ExportImpl", e) + Log.e("ExportImpl", "Failed to export songs to JSON", e)🤖 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/data/backup/ExportImpl.kt` around lines 39 - 43, In the catch block that currently uses Log.wtf in ExportImpl (the catch (e: Exception) handler), replace Log.wtf("ExportImpl", e) with a recoverable error log such as Log.e("ExportImpl", "Error exporting backup", e) so the exception and context are recorded without invoking the WTF handler; keep the existing return null behavior to preserve recovery.app/build.gradle.kts (1)
27-28:⚠️ Potential issue | 🔴 CriticalVersion numbers not updated for the 5.6.0 release.
appVersionNameis still"5.5.2"andappVersionCodeis still5520, but this PR is titled "5.6.0" and the changelog contains a 5.6.0 section.🐛 Proposed fix
-val appVersionName = "5.5.2" -val appVersionCode = 5520 +val appVersionName = "5.6.0" +val appVersionCode = 5600🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/build.gradle.kts` around lines 27 - 28, Update the release version constants: set appVersionName to "5.6.0" and bump appVersionCode to 5600 (calculate as 5.6.0 → 5000+600+0 = 5600) so the code symbols appVersionName and appVersionCode reflect the 5.6.0 release.
♻️ Duplicate comments (4)
app/src/main/java/com/shub39/rush/data/backup/RestoreImpl.kt (1)
28-31: Pre-existingkotlin.io.path/ minSdk 24 incompatibility remains unresolved.The import consolidation is a clean improvement, but the underlying risk flagged in the previous review is still present:
createTempFile,outputStream,readText, anddeleteIfExistsall wrapjava.nio.file.*, which requires API 26, while the project'sminSdkis 24. This PR doesn't address it.🤖 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/data/backup/RestoreImpl.kt` around lines 28 - 31, RestoreImpl.kt still imports kotlin.io.path functions (createTempFile, outputStream, readText, deleteIfExists) which rely on java.nio.file (API 26+) and break minSdk 24; replace those usages with Android-compatible java.io/File APIs: use java.io.File.createTempFile(...) or create temp file in context.cacheDir, call File.outputStream() / File.readText() / File.delete() instead of the kotlin.io.path variants, and remove the kotlin.io.path imports so RestoreImpl.kt no longer references java.nio.file.app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt (2)
24-25:⚠️ Potential issue | 🟠 MajorData layer imports from the presentation layer — architectural violation.
getMainTitleandgetMainArtistlive incom.shub39.rush.presentation, yet the data-layer classLrcLibApiimports and calls them. The data layer must not depend on the presentation layer. These are pure string-utility functions with no UI dependencies; they belong in the domain layer (e.g.,com.shub39.rush.domain.util). This was flagged in a previous review and remains unresolved.🤖 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/data/network/LrcLibApi.kt` around lines 24 - 25, LrcLibApi is importing getMainTitle and getMainArtist from the presentation layer (violation); move these pure string-utility functions into the domain layer (e.g., create com.shub39.rush.domain.util.StringUtils or similar), update their package and signatures there, change imports in LrcLibApi to use com.shub39.rush.domain.util.getMainTitle and getMainArtist, and remove any presentation-layer references so the data layer depends only on domain utilities.
135-142:⚠️ Potential issue | 🔴 CriticalBug: Strategy 5 results are computed but never returned.
The result from
queryLyricsWithParamson Line 138 is assigned toresultsbut the function unconditionally falls through toreturn Result.Error(SourceError.Data.NO_RESULTS)on Line 142, discarding the Strategy 5 results entirely. This was flagged in a previous review and remains unresolved.🐛 Proposed fix
// Strategy 5: Try original title if different from cleaned if (cleanedTitle != trackName.trim()) { results = queryLyricsWithParams(trackName = trackName.trim(), artistName = artistName.trim()) .filter { it.syncedLyrics != null || it.plainLyrics != null } + + if (results.isNotEmpty()) return Result.Success(results) } return Result.Error(SourceError.Data.NO_RESULTS)🤖 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/data/network/LrcLibApi.kt` around lines 135 - 142, The strategy-5 search assigns results from queryLyricsWithParams to the local variable results but then always falls through to return Result.Error(SourceError.Data.NO_RESULTS); fix this by checking the newly assigned results (from queryLyricsWithParams with trackName.trim() and artistName.trim()) and returning a successful Result when they contain syncedLyrics/plainLyrics (the same condition used to filter), otherwise continue to the error return; locate symbols cleanedTitle, trackName, artistName, queryLyricsWithParams, results and ensure the function returns the results when non-empty instead of unconditionally returning Result.Error(SourceError.Data.NO_RESULTS).app/src/main/java/com/shub39/rush/presentation/util.kt (1)
136-145:⚠️ Potential issue | 🟠 Major
getMainArtiststill breaks after the first matching separator.The
breakon Line 141 stops iteration after the first matched separator, so secondary separators in the result are never removed (e.g.,"Artist1 feat. Artist2 & Artist3"→ splits on" & "first → returns"Artist1 feat. Artist2"instead of"Artist1"). This was flagged in a previous review and remains unaddressed.🐛 Proposed fix (remove the `break` and take the minimum-index split)
fun getMainArtist(artists: String): String { - var cleaned = artists.trim() - for (separator in artistSeparators) { - if (cleaned.contains(separator, ignoreCase = true)) { - cleaned = cleaned.split(separator, ignoreCase = true, limit = 2)[0] - break - } - } - return cleaned.trim() + val input = artists.trim() + val earliest = artistSeparators + .mapNotNull { sep -> + val idx = input.indexOf(sep, ignoreCase = true) + if (idx >= 0) idx to sep else null + } + .minByOrNull { it.first } + return if (earliest != null) input.substring(0, earliest.first).trim() else input }🤖 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/util.kt` around lines 136 - 145, getMainArtist currently stops after the first matching separator due to the break, so secondary separators in the result remain; update getMainArtist to consider all separators from artistSeparators (do not break) and choose the earliest split (e.g., track the minimal index or shortest left-side substring) so the returned artist is the prefix before the first occurring separator, then trim and return that value.
🧹 Nitpick comments (6)
app/src/main/java/com/shub39/rush/presentation/provideTypography.kt (2)
34-34: Add@FontResto enforce resource-type safety forfont.This tightens call-site correctness for the nullable font resource parameter.
🔧 Suggested refactor
+import androidx.annotation.FontRes @@ -fun provideTypography(font: Int? = R.font.poppins_regular): Typography { +fun provideTypography(`@FontRes` font: Int? = R.font.poppins_regular): Typography {🤖 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/provideTypography.kt` at line 34, The provideTypography function's font parameter should be annotated with `@FontRes` to enforce resource-type safety; update the function signature for provideTypography(font: Int? = R.font.poppins_regular) to add the `@FontRes` annotation on the font parameter and import androidx.annotation.FontRes so callers and the compiler treat the parameter as a font resource (preserving nullable and default behavior).
35-69: Wrap staticFontFamilyinstances withrememberto avoid rebuilding on recomposition.The three flex font objects don't depend on the
fontparameter and remain identical across all calls. Usingrememberprevents unnecessary allocations when the composable recomposes (e.g., when the theme changes inRushTheme.kt).♻️ Suggested refactor
import androidx.compose.runtime.Composable +import androidx.compose.runtime.remember import androidx.compose.ui.text.ExperimentalTextApi @@ `@OptIn`(ExperimentalTextApi::class) `@Composable` fun provideTypography(font: Int? = R.font.poppins_regular): Typography { - val flexFontDisplay = + val flexFontDisplay = remember { FontFamily( Font( resId = R.font.google_sans_flex, @@ ) ) - val flexFontHeadline = + } + val flexFontHeadline = remember { FontFamily( Font( resId = R.font.google_sans_flex, @@ ) ) - val flexFontTitle = + } + val flexFontTitle = remember { FontFamily( Font( resId = R.font.google_sans_flex, @@ ) ) + }🤖 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/provideTypography.kt` around lines 35 - 69, The three static FontFamily instances (flexFontDisplay, flexFontHeadline, flexFontTitle) are rebuilt on every recomposition; wrap each FontFamily creation with remember so they are allocated once and reused — locate the FontFamily(...) constructions that call Font(resId = R.font.google_sans_flex, variationSettings = ...) and change them to use remember { FontFamily(Font(...)) } (or remember(font) if you must depend on an external param) to prevent unnecessary allocations during recomposition.app/src/main/java/com/shub39/rush/data/network/GeniusApi.kt (1)
42-64: IdenticalHttpClientsetup is duplicated acrossGeniusApi,GeniusScraper, andLrcLibApi.The ContentNegotiation, HttpTimeout, Logging, and defaultRequest configuration blocks are copy-pasted verbatim across three classes. Consider extracting a shared builder function (e.g.,
fun buildRushHttpClient(configure: HttpClientConfig<OkHttpConfig>.() -> Unit = {}): HttpClient) in a common location (e.g.,HttpClientExt.kt) so all three classes delegate to it.🤖 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/data/network/GeniusApi.kt` around lines 42 - 64, The HttpClient setup in GeniusApi (the private val client), GeniusScraper and LrcLibApi is duplicated; extract a shared factory like fun buildRushHttpClient(configure: HttpClientConfig<OkHttpConfig>.() -> Unit = {}) in a new HttpClientExt.kt and have each class replace its local HttpClient creation with buildRushHttpClient { /* per-class overrides */ }; the shared builder should install ContentNegotiation(Json{ignoreUnknownKeys=true}), HttpTimeout (socket/request 20_000ms), conditional Logging when BuildConfig.DEBUG, and defaultRequest contentType(Application.Json), while allowing callers to pass additional configuration via the configure lambda.app/src/main/java/com/shub39/rush/data/network/GeniusScraper.kt (1)
47-53:ContentNegotiationwith JSON is unnecessary inGeniusScraper.
GeniusScraperonly reads raw HTML viabody<String>()and never deserializes JSON. Installing the JSON content negotiation plugin adds overhead without benefit.✂️ Proposed fix
private val client by lazy { HttpClient(OkHttp) { - install(ContentNegotiation) { json(json = Json { ignoreUnknownKeys = true }) } - install(HttpTimeout) {🤖 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/data/network/GeniusScraper.kt` around lines 47 - 53, GeniusScraper is installing ContentNegotiation/Json even though it only reads raw HTML via body<String>(); remove the install(ContentNegotiation) { json(...) } block from the HttpClient(OkHttp) builder in GeniusScraper, leaving the HttpTimeout configuration intact (socketTimeoutMillis/requestTimeoutMillis) and any other plugins; also remove any now-unused imports related to ContentNegotiation and Json to keep the file clean.app/src/test/java/LrcLibApiTest.kt (1)
26-30:testInis duplicated verbatim acrossLrcLibApiTest,GeniusApiTest, andScraperTest.Consider extracting it to a shared abstract base class or a top-level utility in the test source set to avoid the triple copy.
♻️ Suggested extraction
Create a shared test utility (e.g.,
TestHelpers.ktin the test source set):// app/src/test/java/TestHelpers.kt import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.runBlocking fun testIn(title: String, block: suspend CoroutineScope.() -> Unit) = runBlocking { println("\n-- $title --") block.invoke(this) println("\n") }Then remove the local
testInfrom each test class and call the top-level function directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/LrcLibApiTest.kt` around lines 26 - 30, Extract the duplicated testIn function into a shared test helper in the test source set and remove the copies from LrcLibApiTest, GeniusApiTest, and ScraperTest; specifically, create a top-level utility (e.g., TestHelpers.kt) that declares fun testIn(title: String, block: suspend CoroutineScope.() -> Unit) = runBlocking { ... } and import kotlinx.coroutines.CoroutineScope and runBlocking there, then delete the local testIn declarations from the three test classes and call the shared testIn function instead (or alternatively move testIn into a common abstract base test class used by those tests).app/src/main/java/com/shub39/rush/data/repository/RushRepository.kt (1)
31-32: Kotlin 2.3.10 satisfies the ≥2.1 requirement forkotlin.time.Clock.The project uses Kotlin 2.3.10 (from
gradle/libs.versions.toml), which meets the minimum requirement. The@ExperimentalTimeannotation is correctly opted into at line 47.For improved testability, consider injecting
Clockas a dependency rather than usingClock.Systemdirectly—this would allow unit tests to control time deterministically when testingdateAdded. This is a low-priority refactor but worth noting for future improvements.🤖 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/data/repository/RushRepository.kt` around lines 31 - 32, The repository currently uses Clock.System directly which makes date/time hard to control in tests; update RushRepository to accept a kotlin.time.Clock as a constructor dependency (e.g., add parameter clock: Clock = Clock.System) and replace direct uses of Clock.System with the injected clock (references: RushRepository class and any method creating dateAdded). Ensure the existing `@OptIn`(ExperimentalTime::class) usage remains and update any call sites or tests that instantiate RushRepository to rely on the default or provide a test Clock for deterministic behavior.
🤖 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/data/backup/ExportImpl.kt`:
- Line 24: Remove the obsolete opt-in import and annotation: delete the import
kotlin.time.ExperimentalTime and remove the `@OptIn`(ExperimentalTime::class)
annotation near ExportImpl (the opt-in is unnecessary because Kotlin 2.3.10
stabilizes Clock/Instant and ExportImpl uses Long dateAdded in the Song data
class and getAllSongs() has no opt-in requirement). Also replace the logging
call Log.wtf("ExportImpl", e) inside the exception handler with
Log.e("ExportImpl", "message", e) (or a similar Log.e usage) so recoverable
exceptions return null without invoking fatal crash behavior.
In `@app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt`:
- Around line 92-143: searchLrcLyrics is wrapped in safeCall but never returns
an HttpResponse (it uses non-local returns) so safeCall's exception mapping is
never used; remove the safeCall wrapper and make searchLrcLyrics a plain suspend
function that relies on queryLyricsWithParams (which already uses runCatching)
for HTTP error handling, i.e., delete the surrounding safeCall { ... } and keep
the existing strategies and early returns inside searchLrcLyrics while leaving
queryLyricsWithParams and its runCatching logic unchanged.
In `@README.md`:
- Line 3: Update the badge image alt text to use the correct platform
capitalization: locate the <img> tag's alt attribute in README.md (the badge
image tag, e.g., the img element for the GitHub badge) and change "github" to
"GitHub" so the alt text matches official branding.
---
Outside diff comments:
In `@app/build.gradle.kts`:
- Around line 27-28: Update the release version constants: set appVersionName to
"5.6.0" and bump appVersionCode to 5600 (calculate as 5.6.0 → 5000+600+0 = 5600)
so the code symbols appVersionName and appVersionCode reflect the 5.6.0 release.
In `@app/src/main/java/com/shub39/rush/data/backup/ExportImpl.kt`:
- Around line 39-43: In the catch block that currently uses Log.wtf in
ExportImpl (the catch (e: Exception) handler), replace Log.wtf("ExportImpl", e)
with a recoverable error log such as Log.e("ExportImpl", "Error exporting
backup", e) so the exception and context are recorded without invoking the WTF
handler; keep the existing return null behavior to preserve recovery.
In `@app/src/main/java/com/shub39/rush/presentation/saved/SavedPage.kt`:
- Around line 98-121: The LargeFlexibleTopAppBar is missing a scrollBehavior
hookup so it won't collapse; define val scrollBehaviour =
TopAppBarDefaults.enterAlwaysScrollBehavior() just before the Scaffold, pass
scrollBehavior = scrollBehaviour into the LargeFlexibleTopAppBar call, and apply
.nestedScroll(scrollBehaviour.nestedScrollConnection) to the Scaffold's modifier
(i.e., use modifier.nestedScroll(scrollBehaviour.nestedScrollConnection)) so the
app bar collapses on scroll.
In `@app/src/main/java/com/shub39/rush/presentation/share/SharePage.kt`:
- Around line 154-156: Wrap the FileOutputStream creation and usage in a Kotlin
use { } block to guarantee closure even if imageBitmap.compress(...) throws;
replace the manual stream creation and stream.close() around the calls to
FileOutputStream(...) / imageBitmap.compress(Bitmap.CompressFormat.PNG, 100,
stream) in SharePage.kt with FileOutputStream(file).use { stream -> ... } so the
stream is always closed; ensure any exceptions propagate or are handled as
before.
---
Duplicate comments:
In `@app/src/main/java/com/shub39/rush/data/backup/RestoreImpl.kt`:
- Around line 28-31: RestoreImpl.kt still imports kotlin.io.path functions
(createTempFile, outputStream, readText, deleteIfExists) which rely on
java.nio.file (API 26+) and break minSdk 24; replace those usages with
Android-compatible java.io/File APIs: use java.io.File.createTempFile(...) or
create temp file in context.cacheDir, call File.outputStream() / File.readText()
/ File.delete() instead of the kotlin.io.path variants, and remove the
kotlin.io.path imports so RestoreImpl.kt no longer references java.nio.file.
In `@app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt`:
- Around line 24-25: LrcLibApi is importing getMainTitle and getMainArtist from
the presentation layer (violation); move these pure string-utility functions
into the domain layer (e.g., create com.shub39.rush.domain.util.StringUtils or
similar), update their package and signatures there, change imports in LrcLibApi
to use com.shub39.rush.domain.util.getMainTitle and getMainArtist, and remove
any presentation-layer references so the data layer depends only on domain
utilities.
- Around line 135-142: The strategy-5 search assigns results from
queryLyricsWithParams to the local variable results but then always falls
through to return Result.Error(SourceError.Data.NO_RESULTS); fix this by
checking the newly assigned results (from queryLyricsWithParams with
trackName.trim() and artistName.trim()) and returning a successful Result when
they contain syncedLyrics/plainLyrics (the same condition used to filter),
otherwise continue to the error return; locate symbols cleanedTitle, trackName,
artistName, queryLyricsWithParams, results and ensure the function returns the
results when non-empty instead of unconditionally returning
Result.Error(SourceError.Data.NO_RESULTS).
In `@app/src/main/java/com/shub39/rush/presentation/util.kt`:
- Around line 136-145: getMainArtist currently stops after the first matching
separator due to the break, so secondary separators in the result remain; update
getMainArtist to consider all separators from artistSeparators (do not break)
and choose the earliest split (e.g., track the minimal index or shortest
left-side substring) so the returned artist is the prefix before the first
occurring separator, then trim and return that value.
---
Nitpick comments:
In `@app/src/main/java/com/shub39/rush/data/network/GeniusApi.kt`:
- Around line 42-64: The HttpClient setup in GeniusApi (the private val client),
GeniusScraper and LrcLibApi is duplicated; extract a shared factory like fun
buildRushHttpClient(configure: HttpClientConfig<OkHttpConfig>.() -> Unit = {})
in a new HttpClientExt.kt and have each class replace its local HttpClient
creation with buildRushHttpClient { /* per-class overrides */ }; the shared
builder should install ContentNegotiation(Json{ignoreUnknownKeys=true}),
HttpTimeout (socket/request 20_000ms), conditional Logging when
BuildConfig.DEBUG, and defaultRequest contentType(Application.Json), while
allowing callers to pass additional configuration via the configure lambda.
In `@app/src/main/java/com/shub39/rush/data/network/GeniusScraper.kt`:
- Around line 47-53: GeniusScraper is installing ContentNegotiation/Json even
though it only reads raw HTML via body<String>(); remove the
install(ContentNegotiation) { json(...) } block from the HttpClient(OkHttp)
builder in GeniusScraper, leaving the HttpTimeout configuration intact
(socketTimeoutMillis/requestTimeoutMillis) and any other plugins; also remove
any now-unused imports related to ContentNegotiation and Json to keep the file
clean.
In `@app/src/main/java/com/shub39/rush/data/repository/RushRepository.kt`:
- Around line 31-32: The repository currently uses Clock.System directly which
makes date/time hard to control in tests; update RushRepository to accept a
kotlin.time.Clock as a constructor dependency (e.g., add parameter clock: Clock
= Clock.System) and replace direct uses of Clock.System with the injected clock
(references: RushRepository class and any method creating dateAdded). Ensure the
existing `@OptIn`(ExperimentalTime::class) usage remains and update any call sites
or tests that instantiate RushRepository to rely on the default or provide a
test Clock for deterministic behavior.
In `@app/src/main/java/com/shub39/rush/presentation/provideTypography.kt`:
- Line 34: The provideTypography function's font parameter should be annotated
with `@FontRes` to enforce resource-type safety; update the function signature for
provideTypography(font: Int? = R.font.poppins_regular) to add the `@FontRes`
annotation on the font parameter and import androidx.annotation.FontRes so
callers and the compiler treat the parameter as a font resource (preserving
nullable and default behavior).
- Around line 35-69: The three static FontFamily instances (flexFontDisplay,
flexFontHeadline, flexFontTitle) are rebuilt on every recomposition; wrap each
FontFamily creation with remember so they are allocated once and reused — locate
the FontFamily(...) constructions that call Font(resId =
R.font.google_sans_flex, variationSettings = ...) and change them to use
remember { FontFamily(Font(...)) } (or remember(font) if you must depend on an
external param) to prevent unnecessary allocations during recomposition.
In `@app/src/test/java/LrcLibApiTest.kt`:
- Around line 26-30: Extract the duplicated testIn function into a shared test
helper in the test source set and remove the copies from LrcLibApiTest,
GeniusApiTest, and ScraperTest; specifically, create a top-level utility (e.g.,
TestHelpers.kt) that declares fun testIn(title: String, block: suspend
CoroutineScope.() -> Unit) = runBlocking { ... } and import
kotlinx.coroutines.CoroutineScope and runBlocking there, then delete the local
testIn declarations from the three test classes and call the shared testIn
function instead (or alternatively move testIn into a common abstract base test
class used by those tests).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/src/main/res/font/google_sans.ttfis excluded by!**/*.ttfapp/src/main/res/font/google_sans_flex.ttfis excluded by!**/*.ttf
📒 Files selected for processing (31)
CHANGELOG.mdREADME.mdapp/build.gradle.ktsapp/src/main/java/com/shub39/rush/data/HttpClientFactory.ktapp/src/main/java/com/shub39/rush/data/backup/ExportImpl.ktapp/src/main/java/com/shub39/rush/data/backup/RestoreImpl.ktapp/src/main/java/com/shub39/rush/data/network/GeniusApi.ktapp/src/main/java/com/shub39/rush/data/network/GeniusScraper.ktapp/src/main/java/com/shub39/rush/data/network/LrcLibApi.ktapp/src/main/java/com/shub39/rush/data/network/Tokens.ktapp/src/main/java/com/shub39/rush/data/repository/RushRepository.ktapp/src/main/java/com/shub39/rush/di/RushModules.ktapp/src/main/java/com/shub39/rush/presentation/enumExt.ktapp/src/main/java/com/shub39/rush/presentation/glow.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/CurveVisualizer.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/component/SyncedLyrics.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.ktapp/src/main/java/com/shub39/rush/presentation/provideTypography.ktapp/src/main/java/com/shub39/rush/presentation/saved/SavedPage.ktapp/src/main/java/com/shub39/rush/presentation/setting/component/AboutApp.ktapp/src/main/java/com/shub39/rush/presentation/setting/section/BackupPage.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/java/com/shub39/rush/presentation/util.ktapp/src/main/res/values-frc/strings.xmlapp/src/test/java/GeniusApiTest.ktapp/src/test/java/LrcLibApiTest.ktapp/src/test/java/ScraperTest.ktgradle/libs.versions.tomlvisualizer-helper/build.gradle.kts
💤 Files with no reviewable changes (4)
- app/src/main/java/com/shub39/rush/di/RushModules.kt
- app/src/main/java/com/shub39/rush/data/HttpClientFactory.kt
- visualizer-helper/build.gradle.kts
- app/src/main/java/com/shub39/rush/data/network/Tokens.kt
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt (1)
42-81: Lazy HttpClient initialization is well-configured.The internal lazy initialization keeps the API class self-contained. Configuration looks solid with proper timeouts and conditional debug logging.
One consideration:
expectSuccess = true(line 79) will throw on non-2xx responses, which is caught byrunCatchinginqueryLyricsWithParams. This silently converts server errors to empty results. For production debugging, you might want to log these failures.♻️ Optional: Add error logging for visibility
private suspend fun queryLyricsWithParams( trackName: String? = null, artistName: String? = null, albumName: String? = null, query: String? = null, ): List<LrcGetDto> = runCatching { client .get("/api/search") { if (query != null) parameter("q", query) if (trackName != null) parameter("track_name", trackName) if (artistName != null) parameter("artist_name", artistName) if (albumName != null) parameter("album_name", albumName) } .body<List<LrcGetDto>>() } - .getOrDefault(emptyList()) + .onFailure { e -> + if (BuildConfig.DEBUG) println("LrcLib query failed: ${e.message}") + } + .getOrDefault(emptyList())🤖 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/data/network/LrcLibApi.kt` around lines 42 - 81, The client is configured with expectSuccess = true so non-2xx responses thrown from HTTP calls inside LrcLibApi are being swallowed by the runCatching in queryLyricsWithParams; update queryLyricsWithParams to log the caught exception (include status and message where available) using the project logger (or Android Log) before returning the empty result so server/client errors are visible in production; specifically, in the catch block for queryLyricsWithParams, record the Throwable (and if it's an HttpResponseException or ResponseException extract response.status and response.body/response.message) and emit a clear log entry referencing LrcLibApi.queryLyricsWithParams.app/src/main/java/com/shub39/rush/presentation/ProvideTypography.kt (1)
80-96: Wire flex presets intoprovideTypography()output.The flex preset functions defined on lines 34–74 are only used in the preview (lines 104–106) and not applied to the returned Typography object. Lines 81–89 should use the flex presets for display/headline/title styles instead of
selectedFont.♻️ Proposed refactor
return Typography( - displayLarge = TYPOGRAPHY.displayLarge.copy(fontFamily = selectedFont), - displayMedium = TYPOGRAPHY.displayMedium.copy(fontFamily = selectedFont), - displaySmall = TYPOGRAPHY.displaySmall.copy(fontFamily = selectedFont), - headlineLarge = TYPOGRAPHY.headlineLarge.copy(fontFamily = selectedFont), - headlineMedium = TYPOGRAPHY.headlineMedium.copy(fontFamily = selectedFont), - headlineSmall = TYPOGRAPHY.headlineSmall.copy(fontFamily = selectedFont), - titleLarge = TYPOGRAPHY.titleLarge.copy(fontFamily = selectedFont), - titleMedium = TYPOGRAPHY.titleMedium.copy(fontFamily = selectedFont), - titleSmall = TYPOGRAPHY.titleSmall.copy(fontFamily = selectedFont), + displayLarge = TYPOGRAPHY.displayLarge.copy(fontFamily = flexFontEmphasis()), + displayMedium = TYPOGRAPHY.displayMedium.copy(fontFamily = flexFontEmphasis()), + displaySmall = TYPOGRAPHY.displaySmall.copy(fontFamily = flexFontEmphasis()), + headlineLarge = TYPOGRAPHY.headlineLarge.copy(fontFamily = flexFontBold()), + headlineMedium = TYPOGRAPHY.headlineMedium.copy(fontFamily = flexFontBold()), + headlineSmall = TYPOGRAPHY.headlineSmall.copy(fontFamily = flexFontBold()), + titleLarge = TYPOGRAPHY.titleLarge.copy(fontFamily = flexFontRounded()), + titleMedium = TYPOGRAPHY.titleMedium.copy(fontFamily = flexFontRounded()), + titleSmall = TYPOGRAPHY.titleSmall.copy(fontFamily = flexFontRounded()), bodyLarge = TYPOGRAPHY.bodyLarge.copy(fontFamily = selectedFont), bodyMedium = TYPOGRAPHY.bodyMedium.copy(fontFamily = selectedFont), bodySmall = TYPOGRAPHY.bodySmall.copy(fontFamily = selectedFont),🤖 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/ProvideTypography.kt` around lines 80 - 96, The returned Typography currently applies selectedFont to every style; instead, wire the existing flex preset functions (the flex presets defined earlier that are used in the preview) into the display, headline and title groups: for displayLarge/displayMedium/displaySmall, headlineLarge/headlineMedium/headlineSmall, and titleLarge/titleMedium/titleSmall, call the corresponding flex preset functions instead of using selectedFont so those presets are applied; keep using selectedFont.copy(...) for bodyLarge/bodyMedium/bodySmall and labelLarge/labelMedium/labelSmall as before. Ensure this change is made in provideTypography() where DISPLAY/HEADLINE/TITLE styles are constructed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/shub39/rush/data/network/LrcLibApi.kt`:
- Around line 42-81: The client is configured with expectSuccess = true so
non-2xx responses thrown from HTTP calls inside LrcLibApi are being swallowed by
the runCatching in queryLyricsWithParams; update queryLyricsWithParams to log
the caught exception (include status and message where available) using the
project logger (or Android Log) before returning the empty result so
server/client errors are visible in production; specifically, in the catch block
for queryLyricsWithParams, record the Throwable (and if it's an
HttpResponseException or ResponseException extract response.status and
response.body/response.message) and emit a clear log entry referencing
LrcLibApi.queryLyricsWithParams.
In `@app/src/main/java/com/shub39/rush/presentation/ProvideTypography.kt`:
- Around line 80-96: The returned Typography currently applies selectedFont to
every style; instead, wire the existing flex preset functions (the flex presets
defined earlier that are used in the preview) into the display, headline and
title groups: for displayLarge/displayMedium/displaySmall,
headlineLarge/headlineMedium/headlineSmall, and
titleLarge/titleMedium/titleSmall, call the corresponding flex preset functions
instead of using selectedFont so those presets are applied; keep using
selectedFont.copy(...) for bodyLarge/bodyMedium/bodySmall and
labelLarge/labelMedium/labelSmall as before. Ensure this change is made in
provideTypography() where DISPLAY/HEADLINE/TITLE styles are constructed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/src/main/java/com/shub39/rush/data/backup/ExportImpl.ktapp/src/main/java/com/shub39/rush/data/listener/MediaListenerImpl.ktapp/src/main/java/com/shub39/rush/data/network/LrcLibApi.ktapp/src/main/java/com/shub39/rush/domain/Util.ktapp/src/main/java/com/shub39/rush/presentation/EnumExt.ktapp/src/main/java/com/shub39/rush/presentation/ErrorStringRes.ktapp/src/main/java/com/shub39/rush/presentation/Glow.ktapp/src/main/java/com/shub39/rush/presentation/ListItemExt.ktapp/src/main/java/com/shub39/rush/presentation/ProvideTypography.ktapp/src/main/java/com/shub39/rush/presentation/Scrollbar.ktapp/src/main/java/com/shub39/rush/presentation/Util.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/Util.ktapp/src/main/java/com/shub39/rush/presentation/share/Util.ktapp/src/main/java/com/shub39/rush/viewmodels/SavedVM.ktapp/src/main/java/com/shub39/rush/viewmodels/SearchSheetVM.kt
💤 Files with no reviewable changes (2)
- app/src/main/java/com/shub39/rush/presentation/Util.kt
- app/src/main/java/com/shub39/rush/data/backup/ExportImpl.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/shub39/rush/viewmodels/SearchSheetVM.kt
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsCustomisationPage.kt (1)
131-138: Consider applying consistent styling to both portrait and landscape TopAppBar titles.The non-wide layout now uses
flexFontBold()andbasicMarquee()for the title (lines 131-138), but the wide/landscape layout at line 237 still uses a plainText. If the styling difference is intentional (e.g., landscape has sufficient width), this is fine. Otherwise, consider applying the same treatment for visual consistency.♻️ Optional: Apply same styling to landscape TopAppBar
TopAppBar( - title = { Text(stringResource(R.string.customisations)) }, + title = { + Text( + text = stringResource(R.string.customisations), + fontFamily = flexFontBold(), + maxLines = 1, + modifier = Modifier.basicMarquee(), + ) + }, colors =Also applies to: 236-237
🤖 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/LyricsCustomisationPage.kt` around lines 131 - 138, The wide/landscape TopAppBar title uses a plain Text while the portrait title uses flexFontBold() and Modifier.basicMarquee(), causing inconsistent styling; update the Text used in the wide/landscape TopAppBar (the title lambda in LyricsCustomisationPage.kt for the wide layout) to use the same properties: set fontFamily = flexFontBold(), maxLines = 1, and modifier = Modifier.basicMarquee() (keeping the same stringResource call) so both layouts render the title consistently.app/src/main/java/com/shub39/rush/presentation/saved/SavedPage.kt (1)
106-116: Consider extracting shared top-bar title/subtitle content to avoid drift.The same
Textblocks are duplicated in portrait and landscape branches; hoisting them once improves maintainability.♻️ Proposed refactor
+ val topBarTitle: `@Composable` () -> Unit = { + Text( + text = stringResource(R.string.rush_branding), + fontFamily = flexFontEmphasis(), + ) + } + val topBarSubtitle: `@Composable` () -> Unit = { + Text( + text = "${state.songsAsc.size} " + stringResource(R.string.saved), + fontFamily = flexFontRounded(), + ) + } + Scaffold( modifier = modifier, topBar = { Column { if (!isLandscape) { LargeFlexibleTopAppBar( - title = { - Text( - text = stringResource(R.string.rush_branding), - fontFamily = flexFontEmphasis(), - ) - }, - subtitle = { - Text( - text = "${state.songsAsc.size} " + stringResource(R.string.saved), - fontFamily = flexFontRounded(), - ) - }, + title = topBarTitle, + subtitle = topBarSubtitle, actions = { IconButton(onClick = onNavigateToSettings) { Icon( @@ } else { TopAppBar( - title = { - Text( - text = stringResource(R.string.rush_branding), - fontFamily = flexFontEmphasis(), - ) - }, - subtitle = { - Text( - text = "${state.songsAsc.size} " + stringResource(R.string.saved), - fontFamily = flexFontRounded(), - ) - }, + title = topBarTitle, + subtitle = topBarSubtitle, actions = { IconButton(onClick = onNavigateToSettings) { Icon(Also applies to: 134-144
🤖 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/saved/SavedPage.kt` around lines 106 - 116, Duplicate Text title/subtitle blocks in the SavedPage composable should be hoisted into a single reusable component to avoid drift: create a small `@Composable` (e.g., SavedTopBarTitle or SavedTopBarContent) that accepts the count (state.songsAsc.size) or the entire state and renders the two Text elements (using flexFontEmphasis() and flexFontRounded() and stringResource(R.string.rush_branding)/stringResource(R.string.saved)); then replace the duplicated title = { ... } and subtitle = { ... } lambdas in both portrait and landscape branches with a call to that new composable so both branches reuse the same UI.app/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.kt (1)
107-107: Minor: Consider using consistent American spelling for the variable name.The variable
scrollBehaviouruses British spelling while the Material 3 API uses American spelling (scrollBehavior). For consistency with the framework naming, consider renaming toscrollBehavior.✏️ Suggested rename
- val scrollBehaviour = TopAppBarDefaults.enterAlwaysScrollBehavior() + val scrollBehavior = TopAppBarDefaults.enterAlwaysScrollBehavior()And update references at lines 125 and 129 accordingly.
🤖 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` at line 107, Rename the local variable `scrollBehaviour` to use American spelling `scrollBehavior` to match the Material3 API (the value assigned from TopAppBarDefaults.enterAlwaysScrollBehavior()); update all references to this variable (e.g., uses inside the LookAndFeelPage composable where `scrollBehaviour` is passed or accessed) to `scrollBehavior` so names are consistent with the framework.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsCustomisationPage.kt`:
- Around line 131-138: The wide/landscape TopAppBar title uses a plain Text
while the portrait title uses flexFontBold() and Modifier.basicMarquee(),
causing inconsistent styling; update the Text used in the wide/landscape
TopAppBar (the title lambda in LyricsCustomisationPage.kt for the wide layout)
to use the same properties: set fontFamily = flexFontBold(), maxLines = 1, and
modifier = Modifier.basicMarquee() (keeping the same stringResource call) so
both layouts render the title consistently.
In `@app/src/main/java/com/shub39/rush/presentation/saved/SavedPage.kt`:
- Around line 106-116: Duplicate Text title/subtitle blocks in the SavedPage
composable should be hoisted into a single reusable component to avoid drift:
create a small `@Composable` (e.g., SavedTopBarTitle or SavedTopBarContent) that
accepts the count (state.songsAsc.size) or the entire state and renders the two
Text elements (using flexFontEmphasis() and flexFontRounded() and
stringResource(R.string.rush_branding)/stringResource(R.string.saved)); then
replace the duplicated title = { ... } and subtitle = { ... } lambdas in both
portrait and landscape branches with a call to that new composable so both
branches reuse the same UI.
In
`@app/src/main/java/com/shub39/rush/presentation/setting/section/LookAndFeelPage.kt`:
- Line 107: Rename the local variable `scrollBehaviour` to use American spelling
`scrollBehavior` to match the Material3 API (the value assigned from
TopAppBarDefaults.enterAlwaysScrollBehavior()); update all references to this
variable (e.g., uses inside the LookAndFeelPage composable where
`scrollBehaviour` is passed or accessed) to `scrollBehavior` so names are
consistent with the framework.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
CHANGELOG.mdapp/src/main/java/com/shub39/rush/presentation/Util.ktapp/src/main/java/com/shub39/rush/presentation/components/RushBranding.ktapp/src/main/java/com/shub39/rush/presentation/components/RushTheme.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsCustomisationPage.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.ktapp/src/main/java/com/shub39/rush/presentation/onboarding/Onboarding.ktapp/src/main/java/com/shub39/rush/presentation/saved/SavedPage.ktapp/src/main/java/com/shub39/rush/presentation/setting/SettingsGraph.ktapp/src/main/java/com/shub39/rush/presentation/setting/component/AboutApp.ktapp/src/main/java/com/shub39/rush/presentation/setting/section/BackupPage.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/component/cards/AlbumArt.ktapp/src/main/java/com/shub39/rush/presentation/share/component/cards/ChatCard.ktapp/src/main/java/com/shub39/rush/presentation/share/component/cards/CoupletShareCard.ktapp/src/main/java/com/shub39/rush/presentation/share/component/cards/MessyCard.ktapp/src/main/java/com/shub39/rush/presentation/share/component/cards/QuoteShareCard.ktapp/src/main/java/com/shub39/rush/presentation/share/component/cards/RushedShareCard.ktapp/src/main/java/com/shub39/rush/presentation/share/component/cards/SpotifyShareCard.ktapp/src/main/java/com/shub39/rush/presentation/share/component/cards/VerticalShareCard.ktapp/src/main/res/values-frc/strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/shub39/rush/presentation/setting/SettingsGraph.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/java/com/shub39/rush/presentation/setting/section/SettingRootPage.kt
- app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt
- app/src/main/res/values-frc/strings.xml
- CHANGELOG.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsCustomisationPage.kt (1)
131-138: Consider extracting a shared title composable to prevent layout drift.The narrow layout now uses custom font + marquee, while the wide layout still has a plain title (Line 237). A shared composable keeps style consistent and reduces duplication.
Refactor sketch
+@Composable +private fun CustomisationsTopBarTitle(useMarquee: Boolean) { + Text( + text = stringResource(R.string.customisations), + fontFamily = flexFontEmphasis(), + maxLines = 1, + modifier = if (useMarquee) Modifier.basicMarquee() else Modifier, + ) +} ... - title = { - Text( - text = stringResource(R.string.customisations), - fontFamily = flexFontEmphasis(), - maxLines = 1, - modifier = Modifier.basicMarquee(), - ) - }, + title = { CustomisationsTopBarTitle(useMarquee = true) }, ... - title = { Text(stringResource(R.string.customisations)) }, + title = { CustomisationsTopBarTitle(useMarquee = false) },🤖 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/LyricsCustomisationPage.kt` around lines 131 - 138, Extract the repeated title Text into a shared composable (e.g., LyricsPageTitle or CustomisationTitle) that wraps Text(... text = stringResource(R.string.customisations) ...) and applies flexFontEmphasis(), maxLines = 1 and Modifier.basicMarquee() (accepting an optional Modifier parameter), then replace the inline title lambdas in LyricsCustomisationPage (the narrow layout usage shown) and the wide-layout title (the other occurrence) with calls to this new composable so both layouts share identical styling and avoid layout drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsCustomisationPage.kt`:
- Around line 131-138: Extract the repeated title Text into a shared composable
(e.g., LyricsPageTitle or CustomisationTitle) that wraps Text(... text =
stringResource(R.string.customisations) ...) and applies flexFontEmphasis(),
maxLines = 1 and Modifier.basicMarquee() (accepting an optional Modifier
parameter), then replace the inline title lambdas in LyricsCustomisationPage
(the narrow layout usage shown) and the wide-layout title (the other occurrence)
with calls to this new composable so both layouts share identical styling and
avoid layout drift.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
fastlane/metadata/android/en-US/images/phoneScreenshots/1.pngis excluded by!**/*.pngfastlane/metadata/android/en-US/images/phoneScreenshots/2.pngis excluded by!**/*.pngfastlane/metadata/android/en-US/images/phoneScreenshots/3.pngis excluded by!**/*.pngfastlane/metadata/android/en-US/images/phoneScreenshots/4.pngis excluded by!**/*.pngfastlane/metadata/android/en-US/images/phoneScreenshots/5.pngis excluded by!**/*.pngfastlane/metadata/android/en-US/images/phoneScreenshots/6.pngis excluded by!**/*.png
📒 Files selected for processing (2)
app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsCustomisationPage.ktapp/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/shub39/rush/presentation/lyrics/section/LyricsPage.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.
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Tests