Feature/devtools - #51
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a gated developer tools suite: Timber → DevToolsTimberTree → thread-safe DevToolsLogBuffer (StateFlow) → Compose DevToolsOverlay with Logs/Player/DB/Tools panels, DI providers, developer-mode toggles (Easter egg + settings), DB/playback inspection helpers, resources, and small build/docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Timber as Timber
participant DevTree as DevToolsTimberTree
participant Buffer as DevToolsLogBuffer
participant State as StateFlow<List<DevToolsLog>>
participant UI as DevToolsOverlay UI
App->>Timber: log(priority, tag, message)
Timber->>DevTree: dispatch log(...)
DevTree->>DevTree: construct DevToolsLog
DevTree->>Buffer: add(log)
Buffer->>Buffer: ring-buffer insert (locked)
Buffer->>State: emit(snapshot)
State-->>UI: collectAsState() updates
UI->>UI: render LogViewerPanel / other panels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive suite of developer tools to the Metrolist application, addressing the previous lack of built-in debugging utilities. The new tools provide developers and advanced users with the ability to inspect the app's internal state, view real-time logs, monitor playback, and manage cached data directly from a floating overlay. This significantly enhances troubleshooting capabilities for runtime issues, playback failures, and synchronization problems, improving the overall development and maintenance workflow. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
WHY DID IT ASSIGN NYX |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of developer tools, including a log viewer, playback state inspector, database info panel, and various utility actions. The implementation is well-structured, leveraging dependency injection, coroutines, and StateFlow for a reactive UI. The code is clean and includes good practices like thread-safe log buffering and redacting sensitive information in log exports. I've added a couple of minor suggestions for consistency and code style.
| - New features: `feat/short-description` | ||
| - Refactoring: `ref/short-description` |
There was a problem hiding this comment.
The branch naming convention for features has been updated from feature/short-description to feat/short-description. This pull request's branch name appears to be Feature/devtools, which doesn't follow this new convention. For consistency, please ensure future branches adhere to the updated guidelines.
| modifier = Modifier.padding(top = 2.dp) | ||
| ) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This file is missing a final newline character. It's a common convention to end files with a newline to prevent issues with file concatenation and to follow POSIX standards. Please add a newline at the end of this file.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23906388cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (discordRpc?.isRpcRunning() == true) { | ||
| discordRpc?.closeRPC() | ||
| Timber.tag(TAG).d("Discord RPC: tearing down previous instance") | ||
| runCatching { discordRpc?.close() } |
There was a problem hiding this comment.
discordRpc?.close() here no longer tears down the websocket: in kizzy/src/main/kotlin/com/my/kizzy/rpc/KizzyRPC.kt, close() only sends an empty presence (and even reconnects if needed), while closeRPC() is the method that actually closes the socket. Because this teardown path now uses close(), disabling/reinitializing Discord RPC can leave old connections alive, which risks leaked network/battery usage and duplicate active sessions over time.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
408-424:⚠️ Potential issue | 🟠 MajorRecreate
discordRpcinstance afterclose()calls—updates silently fail without re-instantiation.Current flow closes the RPC at lines 412 and 2156 but doesn't recreate the instance before calling
updateDiscordRPC()afterward. Meanwhile, the pattern at lines 772–778 shows the right approach:close()→discordRpc = null→discordRpc = DiscordRPC(...). Sinceclose()is terminal, presence updates at lines 423 and 2165+ will silently fail against the closed instance. Match the rebuild pattern from line 778 for both screen state and playback stopped paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 408 - 424, The code closes the Discord RPC via discordRpc?.close() (in the Intent.ACTION_SCREEN_OFF and playback-stopped paths) but never re-initializes discordRpc before calling updateDiscordRPC(), so updates silently fail; modify the handlers (the blocks reacting to Intent.ACTION_SCREEN_OFF, Intent.ACTION_SCREEN_ON and the playback-stopped logic) to follow the existing pattern used around the other RPC lifecycle code: after calling close() set discordRpc = null and then reassign discordRpc = DiscordRPC(...) (or call the shared initializer) before invoking updateDiscordRPC(song), ensuring updateDiscordRPC uses a live DiscordRPC instance; target the discordRpc variable, the close() calls, and the updateDiscordRPC(...) invocations when applying the change.AGENTS.md (1)
39-43:⚠️ Potential issue | 🟡 MinorBuild step and APK path should be updated together.
After switching to
assembleFossDebugon Line 39, the install path on Line 43 should point to the Foss debug artifact, notuniversalFoss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 39 - 43, The install path still points to the universalFoss APK after you changed the build command to assembleFossDebug; update the installation path text so it references the Foss debug artifact that assembleFossDebug produces (replace references to app-universal-foss-debug.apk/universalFoss with the corresponding app-foss-debug.apk/foss debug artifact) so the build step and APK path match (look for the lines mentioning assembleFossDebug and app-universal-foss-debug.apk).
🧹 Nitpick comments (8)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
410-414: Centralize Discord RPC teardown into one helper to avoid lifecycle drift.Same close/log/runCatching pattern is repeated in multiple branches with slightly different semantics. A single helper keeps behavior consistent and easier to harden.
Refactor sketch
+ private fun closeDiscordRpc(reason: String, clearReference: Boolean = false) { + val rpc = discordRpc + if (clearReference) discordRpc = null + if (rpc == null) return + Timber.tag(TAG).d("Discord RPC: %s", reason) + scope.launch(Dispatchers.IO) { + runCatching { rpc.close() } + .onFailure { Timber.tag(TAG).e(it, "Failed to close Discord RPC") } + } + }Then replace repeated blocks with calls like:
closeDiscordRpc("screen off while paused, closing connection")closeDiscordRpc("tearing down previous instance", clearReference = true)closeDiscordRpc("playback stopped, closing presence")closeDiscordRpc("service destroying, closing RPC", clearReference = true)Also applies to: 771-773, 2154-2158, 3129-3135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 410 - 414, Multiple places repeat the same runCatching + scope.launch(Dispatchers.IO) { discordRpc?.close() } with slightly different logging/cleanup semantics; create a single helper function closeDiscordRpc(message: String, clearReference: Boolean = false) that performs scope.launch(Dispatchers.IO) { Timber.tag(TAG).d(message); runCatching { discordRpc?.close() }.onFailure { Timber.tag(TAG).e(it, "Failed to close Discord RPC") }; if (clearReference) discordRpc = null } and replace all occurrences (e.g., the blocks around Timber.tag(TAG).d("Discord RPC: screen off while paused, closing connection"), the blocks at the other mentioned locations, and any teardown in onDestroy/stop/stopPlayback) with calls to closeDiscordRpc(...) using appropriate message and clearReference where callers previously cleared the reference.app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt (1)
154-157: Avoid rendering an empty-valueInfoRowfor the queue header.Using
InfoRow(..., "")creates a blank value field. A label-only text row (or nullable-value overload) is cleaner and more accessible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt` around lines 154 - 157, Replace the header call that renders an empty value field (InfoRow(stringResource(R.string.dev_queue_viewer_subtitle), "")) with a label-only variant: call the existing InfoRow overload that accepts only a label (e.g., InfoRow(stringResource(R.string.dev_queue_viewer_subtitle))) or pass a nullable value (e.g., value = null) so the component renders a label-only row; if no such overload exists, add a nullable-value overload to InfoRow to render the label without an empty value, leaving the upcomingItems.forEachIndexed { ... } loop unchanged.app/src/main/kotlin/com/metrolist/music/MainActivity.kt (1)
463-464: Consider single-source gating for DevTools mode.
DevToolsOverlayalready short-circuits on dev mode off, so keeping a second gate here is redundant. You can simplify by composing overlay directly and letting it own the guard.Also applies to: 1149-1154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/MainActivity.kt` around lines 463 - 464, Remove the redundant outer dev-mode guard: delete the local val devMode by rememberPreference(DeveloperModeKey, defaultValue = false) and any surrounding if (devMode) { ... } that conditionally composes DevToolsOverlay; instead always compose DevToolsOverlay directly since DevToolsOverlay itself short-circuits when dev mode is off. Do the same change for the other occurrence that mirrors this pattern (the block referencing DeveloperModeKey at the other location) so DevToolsOverlay is only gated internally.app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt (1)
155-156: Tab selection resets on configuration change.
mutableIntStateOf(0)withremembermeans the selected tab resets to "Logs" whenever the dialog is recreated (e.g., rotation). If this is intentional (fresh start each time), that's fine. If continuity is preferred, considerrememberSaveable.💡 Optional: Persist tab selection across config changes
- var selectedTab by remember { mutableIntStateOf(0) } + var selectedTab by rememberSaveable { mutableIntStateOf(0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt` around lines 155 - 156, The selectedTab state currently uses remember { mutableIntStateOf(0) } which resets on configuration changes; replace it with a saved state using rememberSaveable (e.g., selectedTab by rememberSaveable { mutableIntStateOf(0) }) so the tab selection persists across rotations and dialog recreation while leaving the tabs list and stringResource usage unchanged.app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt (1)
35-36:isDbOpenis captured once and won't update reactively.The
remember { database.isOpen }captures the connection status at initial composition only. If the database connection changes during the overlay's lifetime, this value becomes stale—potentially misleading for a debugging tool.Consider making this reactive if the underlying
MusicDatabase.isOpencan change:♻️ If connection state can change, derive it reactively
val dbName = remember { database.openHelper.databaseName ?: "Unknown" } - val isDbOpen = remember { database.isOpen } + val isDbOpen = database.isOpen // Re-read on each recompositionOr if you want to avoid recomposition overhead and
isOpenis unlikely to change:- val isDbOpen = remember { database.isOpen } + // Note: Connection status is captured once; restart overlay to refresh + val isDbOpen = remember { database.isOpen }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt` around lines 35 - 36, The isDbOpen value is captured once with remember { database.isOpen } and won't update; make it reactive by replacing it with a derived state that reads database.isOpen (e.g., use remember { derivedStateOf { database.isOpen } } or remember { derivedStateOf { ... } } with delegated 'by') inside DatabaseInfoPanel so UI recomposes when MusicDatabase.isOpen changes, and update subsequent uses to read the derived state's value instead of the stale boolean.app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (2)
97-98: Integer division loses precision for RAM values.Using
/ 1024 / 1024 / 1024withLongvalues truncates fractional gigabytes. A device with 7.8GB RAM shows as "7GB available of 7GB total".💡 Show decimal precision for RAM stats
- val totalRamGb = memInfo.totalMem / 1024 / 1024 / 1024 - val availRamGb = memInfo.availMem / 1024 / 1024 / 1024 + val totalRamGb = String.format("%.1f", memInfo.totalMem / 1024.0 / 1024.0 / 1024.0) + val availRamGb = String.format("%.1f", memInfo.availMem / 1024.0 / 1024.0 / 1024.0)Note: You'll need to update the string resource format specifier from
%sto handle the new format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt` around lines 97 - 98, The current calculation for totalRamGb and availRamGb uses integer division (memInfo.totalMem / 1024 / 1024 / 1024) which truncates decimals; change those to perform floating-point division (e.g., memInfo.totalMem.toDouble() / 1024.0 / 1024.0 / 1024.0) and store as Double (or Float) so fractional GB are preserved, then format the displayed values with a decimal format (like one decimal place) when building the UI string; also update the associated string resource format specifier from a string placeholder (%s) to a numeric format (e.g., %.1f or %,.1f) so the formatted decimal GB values render correctly.
163-165: Silent failure on partial cache deletion.The
allDeletedflag is set but never surfaced to the user. If some cache files can't be deleted (e.g., in use), the user sees "Cleared X MB" even though cleanup was incomplete.💡 Optional: Inform user of partial failure
files?.forEach { if (!it.deleteRecursively()) allDeleted = false } - if (!allDeleted) { - // Some files couldn't be deleted, but we continue - } - size / 1024 / 1024 + Pair(size / 1024 / 1024, allDeleted) } - Toast.makeText(context, context.getString(R.string.cleared_cache_mb, sizeMb), Toast.LENGTH_SHORT).show() + val (sizeMb, allDeleted) = result + val message = if (allDeleted) { + context.getString(R.string.cleared_cache_mb, sizeMb) + } else { + context.getString(R.string.cleared_cache_mb_partial, sizeMb) + } + Toast.makeText(context, message, Toast.LENGTH_SHORT).show()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt` around lines 163 - 165, The code currently swallows partial failures when deleting cache: the allDeleted boolean is set but never reported; update the ActionsPanel logic so that after the deletion loop (the block using the allDeleted flag) you surface a warning to the user when allDeleted is false instead of silently continuing. Concretely, modify the code path that currently displays the success message (the same place that shows "Cleared X MB") to append or replace it with a partial-failure message (e.g., "Cleared X MB — some files could not be deleted") or show a modal/notification when allDeleted == false; reference the allDeleted flag and the success-message/display routine in ActionsPanel.kt to locate where to change the UI text/notification. Ensure the message is user-facing and not just logged.app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
15-16: Unused imports detected.
ExperimentalFoundationApiandcombinedClickableare imported but not utilized in this file. The code only uses the standardclickablemodifier at line 390.🧹 Remove unused imports
-import androidx.compose.foundation.ExperimentalFoundationApi -import androidx.compose.foundation.combinedClickable🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt` around lines 15 - 16, Remove the unused imports ExperimentalFoundationApi and combinedClickable from LogViewerPanel.kt: locate the import statements referencing ExperimentalFoundationApi and combinedClickable at the top of the LogViewerPanel file and delete them so only the actually used modifiers (e.g., clickable) remain; this cleans up unused symbols and resolves the warning without changing any runtime 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/kotlin/com/metrolist/music/App.kt`:
- Around line 74-78: The devToolsTimberTree is being planted unconditionally
which causes release builds to capture potentially sensitive logs; update the
code around Timber.plant(devToolsTimberTree) to only plant devToolsTimberTree
when an explicit runtime developer opt-in flag is set (e.g., a new boolean such
as isDevToolsEnabled) and ensure the tree implementation (devToolsTimberTree)
redacts sensitive fields before buffering/exporting logs; additionally, prevent
planting in release builds by checking BuildConfig.DEBUG or the opt-in flag
(whichever you choose) so that release users cannot have logs captured unless
explicitly enabled.
In `@app/src/main/kotlin/com/metrolist/music/devtools/DevToolsTimberTree.kt`:
- Around line 14-22: DevToolsTimberTree currently swallows DebugTree output and
buffers every log unconditionally; change the class to accept an injected
capture gate (e.g., a () -> Boolean like isDeveloperModeEnabled) and update the
log(priority: Int, tag: String?, message: String, t: Throwable?) implementation
to first delegate to Android's DebugTree / call super.log(...) or forward to
Timber.DebugTree so Logcat remains visible in non-dev builds, and only
create/add a DevToolsLog to buffer when the capture gate returns true; also
update AppModule to provide that capture gate (wired to DeveloperModeKey
preference) and plant the tree only when both the app is in a build that should
plant this tree and the provided gate allows capturing.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`:
- Around line 59-88: PanelHeader currently uses the removed
ExperimentalMaterial3ExpressiveApi's LinearWavyProgressIndicator which breaks
builds on Material3 ≥1.4.0; replace it with a stable indicator (e.g.,
LinearProgressIndicator or CircularProgressIndicator) and remove the
ExperimentalMaterial3ExpressiveApi opt-in and any imports referencing it, or
alternatively pin the Material3 dependency to a pre-1.4.0 release if you must
keep LinearWavyProgressIndicator; update PanelHeader to call the chosen stable
composable (maintain the same modifier and placement) and remove the
`@OptIn`(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class)
usage.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 429-439: The IconButton touch target is set to 28.dp which is
below the 48.dp accessibility recommendation; update the IconButton modifier in
the block that calls onToggleSelect(log.id) to ensure a minimum interactive size
(for example replace Modifier.size(28.dp) with
Modifier.size(28.dp).minimumInteractiveComponentSize() or remove the custom size
to use the default IconButton) while keeping the inner Icon at
Modifier.size(20.dp) so the visible icon remains compact; adjust imports if
necessary for minimumInteractiveComponentSize and keep the isSelected logic and
painterResource usage unchanged.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 3128-3138: The teardown currently only calls close() when
discordRpc?.isRpcRunning() == true and otherwise just nulls discordRpc, which
can leak resources; change the logic so any non-null discordRpc is closed (using
scope.launch(Dispatchers.IO) and runCatching { rpc.close() } with
Timber.tag(TAG).e onFailure) before setting discordRpc = null, while keeping the
debug log for the running case and reusing the same rpc variable to avoid races.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt`:
- Around line 95-101: The SwitchPreference toggling devMode (checked = devMode,
onCheckedChange = { devMode = it }) causes the screen to immediately re-render
into the "dev mode required" view, making the toggle disappear and preventing
easy re-enabling; update the onCheckedChange handler in DevToolsSettingsScreen
so that when the new value is false you perform an immediate navigation back
(e.g., call navController.popBackStack() or invoke a provided
onNavigateBack/onClose lambda) instead of just setting devMode, or alternatively
always render the SwitchPreference regardless of devMode (keep the
SwitchPreference component visible while other parts show the required message)
so the user can re-enable from this screen—modify the SwitchPreference
onCheckedChange and surrounding conditional rendering to implement your chosen
approach.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 116-123: The documentation for maskToken says “first 4 and last 2”
but the implementation returns 3+3; update the implementation in function
maskToken to match the docs by returning the first 4 and last 2 characters (use
token.take(4) and token.takeLast(2)) and adjust the short-token guard (e.g.,
return "****" when token.length <= 6) so very short tokens aren’t partially
revealed.
In `@app/src/main/res/values/metrolist_strings.xml`:
- Line 920: Replace the single string resource "dev_mode_taps_remaining" with a
<plurals> resource that provides singular and plural forms (e.g., one: "1 tap to
enable developer mode", other: "%d taps to enable developer mode") and update
all call sites that currently use getString(R.string.dev_mode_taps_remaining, n)
to use getQuantityString(R.plurals.dev_mode_taps_remaining, n, n) (or
Resources.getQuantityString/Context.getResources().getQuantityString as
appropriate) so the UI shows the correct singular/plural form.
In `@development_guide.md`:
- Around line 23-24: The doc shows a mismatch: the build command uses
assembleFossDebug but the APK path references the universalFoss artifact; update
the output path to match the assembleFossDebug artifact (replace the
app-universal-foss-debug.apk reference with the foss variant, e.g.
app/build/outputs/apk/foss/debug/app-foss-debug.apk) or alternatively change the
Gradle task to the universal variant if you intended to keep the universal path
(adjust the assembleFossDebug command accordingly); ensure the two lines
reference the same build variant (assembleFossDebug vs
assembleUniversalFossDebug) and corresponding APK filename.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 39-43: The install path still points to the universalFoss APK
after you changed the build command to assembleFossDebug; update the
installation path text so it references the Foss debug artifact that
assembleFossDebug produces (replace references to
app-universal-foss-debug.apk/universalFoss with the corresponding
app-foss-debug.apk/foss debug artifact) so the build step and APK path match
(look for the lines mentioning assembleFossDebug and
app-universal-foss-debug.apk).
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 408-424: The code closes the Discord RPC via discordRpc?.close()
(in the Intent.ACTION_SCREEN_OFF and playback-stopped paths) but never
re-initializes discordRpc before calling updateDiscordRPC(), so updates silently
fail; modify the handlers (the blocks reacting to Intent.ACTION_SCREEN_OFF,
Intent.ACTION_SCREEN_ON and the playback-stopped logic) to follow the existing
pattern used around the other RPC lifecycle code: after calling close() set
discordRpc = null and then reassign discordRpc = DiscordRPC(...) (or call the
shared initializer) before invoking updateDiscordRPC(song), ensuring
updateDiscordRPC uses a live DiscordRPC instance; target the discordRpc
variable, the close() calls, and the updateDiscordRPC(...) invocations when
applying the change.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt`:
- Around line 97-98: The current calculation for totalRamGb and availRamGb uses
integer division (memInfo.totalMem / 1024 / 1024 / 1024) which truncates
decimals; change those to perform floating-point division (e.g.,
memInfo.totalMem.toDouble() / 1024.0 / 1024.0 / 1024.0) and store as Double (or
Float) so fractional GB are preserved, then format the displayed values with a
decimal format (like one decimal place) when building the UI string; also update
the associated string resource format specifier from a string placeholder (%s)
to a numeric format (e.g., %.1f or %,.1f) so the formatted decimal GB values
render correctly.
- Around line 163-165: The code currently swallows partial failures when
deleting cache: the allDeleted boolean is set but never reported; update the
ActionsPanel logic so that after the deletion loop (the block using the
allDeleted flag) you surface a warning to the user when allDeleted is false
instead of silently continuing. Concretely, modify the code path that currently
displays the success message (the same place that shows "Cleared X MB") to
append or replace it with a partial-failure message (e.g., "Cleared X MB — some
files could not be deleted") or show a modal/notification when allDeleted ==
false; reference the allDeleted flag and the success-message/display routine in
ActionsPanel.kt to locate where to change the UI text/notification. Ensure the
message is user-facing and not just logged.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt`:
- Around line 35-36: The isDbOpen value is captured once with remember {
database.isOpen } and won't update; make it reactive by replacing it with a
derived state that reads database.isOpen (e.g., use remember { derivedStateOf {
database.isOpen } } or remember { derivedStateOf { ... } } with delegated 'by')
inside DatabaseInfoPanel so UI recomposes when MusicDatabase.isOpen changes, and
update subsequent uses to read the derived state's value instead of the stale
boolean.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`:
- Around line 155-156: The selectedTab state currently uses remember {
mutableIntStateOf(0) } which resets on configuration changes; replace it with a
saved state using rememberSaveable (e.g., selectedTab by rememberSaveable {
mutableIntStateOf(0) }) so the tab selection persists across rotations and
dialog recreation while leaving the tabs list and stringResource usage
unchanged.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 15-16: Remove the unused imports ExperimentalFoundationApi and
combinedClickable from LogViewerPanel.kt: locate the import statements
referencing ExperimentalFoundationApi and combinedClickable at the top of the
LogViewerPanel file and delete them so only the actually used modifiers (e.g.,
clickable) remain; this cleans up unused symbols and resolves the warning
without changing any runtime behavior.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt`:
- Around line 154-157: Replace the header call that renders an empty value field
(InfoRow(stringResource(R.string.dev_queue_viewer_subtitle), "")) with a
label-only variant: call the existing InfoRow overload that accepts only a label
(e.g., InfoRow(stringResource(R.string.dev_queue_viewer_subtitle))) or pass a
nullable value (e.g., value = null) so the component renders a label-only row;
if no such overload exists, add a nullable-value overload to InfoRow to render
the label without an empty value, leaving the upcomingItems.forEachIndexed { ...
} loop unchanged.
In `@app/src/main/kotlin/com/metrolist/music/MainActivity.kt`:
- Around line 463-464: Remove the redundant outer dev-mode guard: delete the
local val devMode by rememberPreference(DeveloperModeKey, defaultValue = false)
and any surrounding if (devMode) { ... } that conditionally composes
DevToolsOverlay; instead always compose DevToolsOverlay directly since
DevToolsOverlay itself short-circuits when dev mode is off. Do the same change
for the other occurrence that mirrors this pattern (the block referencing
DeveloperModeKey at the other location) so DevToolsOverlay is only gated
internally.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 410-414: Multiple places repeat the same runCatching +
scope.launch(Dispatchers.IO) { discordRpc?.close() } with slightly different
logging/cleanup semantics; create a single helper function
closeDiscordRpc(message: String, clearReference: Boolean = false) that performs
scope.launch(Dispatchers.IO) { Timber.tag(TAG).d(message); runCatching {
discordRpc?.close() }.onFailure { Timber.tag(TAG).e(it, "Failed to close Discord
RPC") }; if (clearReference) discordRpc = null } and replace all occurrences
(e.g., the blocks around Timber.tag(TAG).d("Discord RPC: screen off while
paused, closing connection"), the blocks at the other mentioned locations, and
any teardown in onDestroy/stop/stopPlayback) with calls to closeDiscordRpc(...)
using appropriate message and clearReference where callers previously cleared
the reference.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
AGENTS.mdapp/src/main/kotlin/com/metrolist/music/App.ktapp/src/main/kotlin/com/metrolist/music/MainActivity.ktapp/src/main/kotlin/com/metrolist/music/db/DatabaseDao.ktapp/src/main/kotlin/com/metrolist/music/db/MusicDatabase.ktapp/src/main/kotlin/com/metrolist/music/devtools/DevToolsLog.ktapp/src/main/kotlin/com/metrolist/music/devtools/DevToolsLogBuffer.ktapp/src/main/kotlin/com/metrolist/music/devtools/DevToolsTimberTree.ktapp/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.ktapp/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.ktapp/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.ktapp/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.ktapp/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.ktapp/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.ktapp/src/main/kotlin/com/metrolist/music/di/AppModule.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/SettingsScreen.ktapp/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.ktapp/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.ktapp/src/main/kotlin/com/metrolist/music/utils/Utils.ktapp/src/main/res/values/metrolist_strings.xmlapp/src/main/res/values/values.xmldevelopment_guide.md
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (8)
app/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.kt (1)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (1)
DevToolsSettingsScreen(43-115)
app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt (1)
lastfm/src/main/kotlin/com/metrolist/lastfm/LastFM.kt (1)
updateNowPlaying(119-139)
app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (1)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
ActionCard(245-297)
app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt (1)
app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt (2)
InfoCard(19-34)InfoRow(36-52)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (2)
app/src/main/kotlin/com/metrolist/music/ui/component/IconButton.kt (1)
IconButton(62-95)app/src/main/kotlin/com/metrolist/music/ui/component/Preference.kt (2)
SwitchPreference(183-217)PreferenceEntry(48-105)
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
app/src/main/kotlin/com/metrolist/music/ui/component/IconButton.kt (1)
IconButton(62-95)
app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt (4)
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
LogViewerPanel(80-365)app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt (1)
PlayerStatePanel(35-116)app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt (1)
DatabaseInfoPanel(25-66)app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (1)
ActionsPanel(66-200)
app/src/main/kotlin/com/metrolist/music/MainActivity.kt (1)
app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt (1)
DevToolsOverlay(90-180)
🪛 LanguageTool
AGENTS.md
[style] ~36-~36: Consider shortening or rephrasing this to strengthen your wording.
Context: ...ding and testing your changes 1. After making changes to the code, you should build the app to e...
(MAKE_CHANGES)
🔇 Additional comments (26)
app/src/main/kotlin/com/metrolist/music/utils/Utils.kt (2)
10-10: No action needed for Line 10 import.Line 10 is a valid dependency import for the new logger path used in
reportException.
14-14: Nice move to unified exception logging on Line 14.This keeps throwable details in the app’s centralized log pipeline, which fits the new DevTools flow.
app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt (1)
62-63: Clean passthrough for DB open-state exposure.Line 62-63 safely exposes existing delegate state without changing DB control flow.
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
43-59: Nice hardening on playback-speed handling and time math.Line 43 and related timing updates prevent invalid speed values from corrupting Discord activity timestamps.
app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt (1)
73-76: Good observability pass for scrobble lifecycle events.The added logging and short-duration guard make troubleshooting way easier without altering core flow semantics.
Also applies to: 86-86, 101-101, 109-109, 121-121, 125-125, 138-138
app/src/main/kotlin/com/metrolist/music/di/AppModule.kt (1)
38-45: DI scope choices look consistent for devtools singletons.Singleton provisioning for buffer/tree and app scope aligns with the runtime usage pattern.
Also applies to: 48-50
app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt (1)
444-448: Async sleep-timer check is a solid callback-path improvement.Moving the timer check off the direct callback path avoids blocking playback state updates.
Also applies to: 546-548
app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLog.kt (1)
10-28: Looks good: log entry model and priority mapping are solid.Clean data shape, sensible defaults, and complete priority labeling for UI rendering.
app/src/main/kotlin/com/metrolist/music/db/DatabaseDao.kt (1)
1730-1740: LGTM for dev stats queries.These DAO additions are clean and directly support the DevTools database panel.
app/src/main/res/values/metrolist_strings.xml (1)
976-1044: Nice coverage on DevTools playback/export labels.The new key set is comprehensive and aligns well with the added overlay panels.
app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLogBuffer.kt (1)
14-60: Good implementation: bounded, thread-safe, and deterministic ordering.The lock + wraparound snapshot logic is clean and matches the devtools use case.
app/src/main/res/values/values.xml (1)
9-11: LGTM on DevTools dimension resources.These constants make the overlay positioning behavior explicit and maintainable.
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/SettingsScreen.kt (1)
55-55: LGTM! Clean integration of DevTools settings entry.The dev mode preference is properly retrieved using the existing
rememberPreferencepattern, and the conditional rendering withinbuildListis a nice approach that keeps the list construction declarative. The navigation route"settings/devtools"correctly matches what's wired inNavigationBuilder.kt.Also applies to: 212-220
app/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.kt (1)
392-394: LGTM! Route registration follows established patterns.The new
"settings/devtools"composable is wired up consistently with other settings screens in this file—same parameter passing, same structure. Good placement in the navigation hierarchy.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (1)
103-110: Crash test handler looks intentional and properly labeled.The deliberate
RuntimeExceptionthrow is clearly marked with error styling (red text/icon), and the description string should communicate the destructive nature. This is a reasonable debugging utility.One small note: in production builds, this could still crash the app if the crash handler isn't properly configured. The commit message mentions "ensured Timber tree is planted in release builds"—might be worth verifying the crash handler setup independently.
app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt (1)
25-65: Overall implementation looks solid.The panel properly uses:
- Locale-aware number formatting
- Reactive Flow collection for counts
- Shared UI components (
PanelHeader,InfoCard,InfoRow)- Appropriate composition patterns
app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt (1)
19-52: LGTM! Clean and reusable shared components.These utility composables (
InfoCard,InfoRow) follow good patterns:
- Proper Material 3 theming integration
- Sensible defaults with customization via modifier
- Monospace font for technical values aids readability in a debugging context
Nice foundation for the DevTools UI system.
app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt (1)
109-110: Good use ofrememberSaveablefor drag offset persistence.The FAB position surviving configuration changes (rotation, etc.) is a nice UX touch. Users won't have to reposition the debug button after rotating their device.
app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (2)
49-64: Solid sensitive data redaction implementation.The regex patterns cover common token/cookie formats (auth tokens, session IDs, visitorData, SAPISID, etc.). Using
$1=<REDACTED>preserves the key name while hiding the value—helpful for debugging what type of data was involved.One consideration: very long base64 tokens might not fully match if they contain characters outside
[\w\-]. But for typical YouTube/Google tokens, this should suffice.
204-251: ActionCard differs from AboutScreen's ActionCard by design.Both files have an
ActionCardcomposable but with different signatures and layouts (this one has a button, the other is fully clickable). This is fine since they serve different interaction patterns—just noting it's not accidental duplication.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (3)
95-97: Nice Easter egg constants—follows familiar Android patterns.9 taps with a 2-second timeout mirrors the "tap build number 7 times" pattern from Android settings. The countdown starting at 3 remaining gives users a hint they're on the right track.
370-396: Well-implemented developer mode Easter egg.The tap-to-unlock logic is clean:
- Timeout resets the counter if taps are too slow
- Early exit if already enabled (no spam)
- Countdown toasts guide the user
- Uses coroutine for dataStore write (non-blocking)
Minor note:
tapCount = 0on line 392 happens inside the coroutine after the write, which is fine since the toast will show before the reset matters.
317-344: Clean migration to Scaffold-based layout.Moving to
Scaffoldwith propertopBarandsnackbarHostslots is a nice improvement over the previous layout structure. The padding is correctly applied to the content column.app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (3)
110-133: Nice background filtering implementation.Running the filter logic on
Dispatchers.DefaultviaproduceStatekeeps the UI thread responsive when processing large log buffers. The filtering criteria (level, tag groups, text search) are cleanly composed.
195-227: Selection bar and clipboard handling look solid.The selection management with copy-to-clipboard functionality is cleanly implemented. Filtering the current
logsstate byselectedLogIdsensures only existing logs are copied, and clearing selection after copy is good UX.
367-378: Clean log row implementation with good visual hierarchy.The priority-to-color mapping and the colored strip indicator provide quick visual identification of log severity. Memoizing the time string and using
SelectionContainerfor copy support are nice touches.
| @OptIn(ExperimentalMaterial3Api::class, androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class) | ||
| @Composable | ||
| fun PanelHeader(title: String, subtitle: String? = null) { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(start = 24.dp, end = 24.dp, top = 24.dp, bottom = 12.dp) | ||
| ) { | ||
| Text( | ||
| text = title, | ||
| style = MaterialTheme.typography.headlineLarge, | ||
| color = MaterialTheme.colorScheme.primary, | ||
| fontWeight = FontWeight.Bold | ||
| ) | ||
| if (subtitle != null) { | ||
| Spacer(modifier = Modifier.height(4.dp)) | ||
| Text( | ||
| text = subtitle, | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant | ||
| ) | ||
| } | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| LinearWavyProgressIndicator( | ||
| progress = { 1f }, | ||
| amplitude = { 1f }, | ||
| modifier = Modifier.fillMaxWidth() | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Material 3 Compose LinearWavyProgressIndicator stable API release status 2025
💡 Result:
In 2025, LinearWavyProgressIndicator in Material 3 for Jetpack Compose did not reach a stable (non-experimental) API release.
- It existed in 1.4.0 alpha builds (e.g., API tweaks like adding
amplitude/waveSpeedwere made while it was still in alpha). [3] - Before the 1.4.0 stable line, Google removed all public APIs tagged with
ExperimentalMaterial3ExpressiveApistarting with 1.4.0-beta01 (July 30, 2025), instructing developers to use 1.5.0-alpha to keep using those “Expressive” APIs. [1] - The underlying change explicitly says it removes the experimental Expressive APIs from compose.material3 in preparation for the 1.4.0 stable release. [2]
- 1.4.0 stable was released September 24, 2025, but these Expressive APIs were already excluded by then. [4]
Bottom line: in 2025, LinearWavyProgressIndicator was not stable; it was effectively alpha/expressive-only and you had to use a pre-beta 1.4 alpha or the 1.5.0-alpha track to access it. [1][2]
Sources: [1] [2] [3] [4]
Swap LinearWavyProgressIndicator for a stable alternative or lock Material 3 to pre-1.4.0 releases.
The ExperimentalMaterial3ExpressiveApi including LinearWavyProgressIndicator was explicitly removed from Material 3 starting with 1.4.0-beta01 (July 2025). This code won't compile against Material 3 1.4.0 stable (released September 2025) or later without forcing alpha dependencies. Even though the wavy progress is a nice touch, this creates a fragile dependency constraint. Either pick a stable progress indicator from Material 3 or document and lock the Material 3 version to pre-1.4.0 alpha.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`
around lines 59 - 88, PanelHeader currently uses the removed
ExperimentalMaterial3ExpressiveApi's LinearWavyProgressIndicator which breaks
builds on Material3 ≥1.4.0; replace it with a stable indicator (e.g.,
LinearProgressIndicator or CircularProgressIndicator) and remove the
ExperimentalMaterial3ExpressiveApi opt-in and any imports referencing it, or
alternatively pin the Material3 dependency to a pre-1.4.0 release if you must
keep LinearWavyProgressIndicator; update PanelHeader to call the chosen stable
composable (maintain the same modifier and placement) and remove the
`@OptIn`(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class)
usage.
| <string name="select_log">Select Log</string> | ||
| <!-- DevTools --> | ||
| <string name="dev_mode_already_enabled">Developer mode is already enabled</string> | ||
| <string name="dev_mode_taps_remaining">%d taps to enable developer mode</string> |
There was a problem hiding this comment.
Use a plural resource for taps remaining.
%d taps to enable developer mode will render awkwardly for singular values (e.g., 1 tap). Convert this to <plurals> and use quantity-aware retrieval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values/metrolist_strings.xml` at line 920, Replace the
single string resource "dev_mode_taps_remaining" with a <plurals> resource that
provides singular and plural forms (e.g., one: "1 tap to enable developer mode",
other: "%d taps to enable developer mode") and update all call sites that
currently use getString(R.string.dev_mode_taps_remaining, n) to use
getQuantityString(R.plurals.dev_mode_taps_remaining, n, n) (or
Resources.getQuantityString/Context.getResources().getQuantityString as
appropriate) so the UI shows the correct singular/plural form.
|
Okay who started an AI review party oh my god |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
770-776:⚠️ Potential issue | 🟠 MajorClose previous
discordRpcunconditionally, and off the collector thread.Line 770 still gates cleanup on
isRpcRunning(). If instance exists but is not “running”, Line 775 drops the reference without a close attempt. Also, Line 772 executes in the collector context, which can stall main-thread work if close is heavy.Proposed fix
- if (discordRpc?.isRpcRunning() == true) { - Timber.tag(TAG).d("Discord RPC: tearing down previous instance") - runCatching { discordRpc?.closeRPC() } - .onFailure { Timber.tag(TAG).e(it, "Failed to close Discord RPC") } - } - discordRpc = null + discordRpc?.let { rpc -> + Timber.tag(TAG).d("Discord RPC: tearing down previous instance") + discordRpc = null + scope.launch(Dispatchers.IO) { + runCatching { rpc.closeRPC() } + .onFailure { Timber.tag(TAG).e(it, "Failed to close Discord RPC") } + } + } ?: run { + discordRpc = null + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 770 - 776, Currently cleanup is gated on discordRpc?.isRpcRunning() and runs on the collector thread; change it to unconditionally attempt to close any existing discordRpc (if discordRpc != null) and perform the close off the collector thread (e.g., launch or withContext(Dispatchers.IO/Default)) using runCatching { discordRpc?.closeRPC() }. After the background close completes (or fails), log failures with Timber.tag(TAG).e(...), then set discordRpc = null; make sure you reference the existing discordRpc, isRpcRunning(), and closeRPC() symbols when implementing this change.
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
254-255: Prefer read-only debug state exposure instead of exposing mutable runtime objects.
internalexposure ofLoudnessEnhancer,DiscordRPC, andScrobbleManagerexpands the mutation surface across the module. For DevTools, a read-only snapshot/StateFlow is safer and keeps lifecycle ownership insideMusicService.Also applies to: 363-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 254 - 255, The service currently exposes mutable runtime objects (LoudnessEnhancer, DiscordRPC, ScrobbleManager and mutable vars like crossfadeEnabled) as internal; instead make the actual instances and mutable vars private inside MusicService and publish a read-only snapshot for DevTools (e.g., a single DebugState data class or separate StateFlows) — create MutableStateFlow/MutableSharedFlow properties (for example _debugState, _crossfadeEnabled) updated by the service and expose only the read-only StateFlow/Flow/LiveData (debugState, crossfadeEnabled) so consumers can observe state without mutating lifecycles or instances; replace internal declarations of LoudnessEnhancer/DiscordRPC/ScrobbleManager with private instances and add lightweight read-only references in the exposed DebugState/StateFlow to represent their statuses instead of exposing the objects themselves.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (1)
108-114: Add a confirmation step before forced crash.One tap currently hard-crashes the app. For a settings surface, a confirm dialog would reduce accidental crashes while keeping the test path available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt` around lines 108 - 114, The PreferenceEntry currently throws a RuntimeException directly in its onClick (the “Developer Triggered Crash” path); replace this with a confirmation dialog flow: add a composable state (e.g., showConfirmCrash : MutableState<Boolean>) in DevToolsSettingsScreen, change the PreferenceEntry onClick to set showConfirmCrash = true, and render a Material3 AlertDialog when showConfirmCrash is true with Cancel (sets showConfirmCrash = false) and Confirm (throws the same RuntimeException) actions; keep the existing message/title (stringResource R.string.test_crash_handler / R.string.test_crash_handler_desc) and the exception text unchanged so the crash behavior remains identical after confirmation.
🤖 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/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 117-124: The tag-matching branch in LogViewerPanel.kt currently
uses else -> true which causes any unrecognized selectedTagGroups to bypass
filtering; change the fallback to else -> false so unknown/localized values do
not match log entries, and then refactor selectedTagGroups to use a stable
internal representation (e.g., a TagGroup enum or sealed class) mapped to
localized labels in the UI so the when { tagPlayer, tagUi, tagDb, tagIntegration
} checks match against stable identifiers rather than raw localized strings.
In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`:
- Around line 388-393: The coroutine launches a suspend write and toast while
tapCount is only reset inside that coroutine, allowing rapid extra taps to queue
duplicate activations; update the logic in the AboutScreen tap handler so you
clear or lock tapCount immediately before starting the suspend work (e.g., set
tapCount = 0 or flip an isEnablingDevMode flag synchronously just prior to
calling coroutineScope.launch), then perform
context.dataStore.edit(DeveloperModeKey) and Toast.makeText inside the
coroutine; reference the existing coroutineScope.launch, tapCount,
DeveloperModeKey, context.dataStore.edit and Toast.makeText when making this
change.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 26-28: The init block in DiscordRPC currently logs a masked token
via Timber.d("DiscordRPC initialized (token=%s)", maskToken(token)); remove any
token-derived output and instead log only the initialization state (e.g.
Timber.d("DiscordRPC initialized")); update the init block and any other uses of
maskToken/token in this class so no secret fragments are emitted and ensure
maskToken(token) is not used anywhere in DiscordRPC.
- Around line 108-110: The override of close() currently calls super.close()
(which only clears presence) but does not tear down the WebSocket; update the
close() implementation to also call closeRPC() (which invokes
discordWebSocket.close()) so the socket is closed and resources aren’t
leaked—either call closeRPC() before/after super.close() or replace the body
with a combined teardown that calls both super.close() and closeRPC().
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 770-776: Currently cleanup is gated on discordRpc?.isRpcRunning()
and runs on the collector thread; change it to unconditionally attempt to close
any existing discordRpc (if discordRpc != null) and perform the close off the
collector thread (e.g., launch or withContext(Dispatchers.IO/Default)) using
runCatching { discordRpc?.closeRPC() }. After the background close completes (or
fails), log failures with Timber.tag(TAG).e(...), then set discordRpc = null;
make sure you reference the existing discordRpc, isRpcRunning(), and closeRPC()
symbols when implementing this change.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 254-255: The service currently exposes mutable runtime objects
(LoudnessEnhancer, DiscordRPC, ScrobbleManager and mutable vars like
crossfadeEnabled) as internal; instead make the actual instances and mutable
vars private inside MusicService and publish a read-only snapshot for DevTools
(e.g., a single DebugState data class or separate StateFlows) — create
MutableStateFlow/MutableSharedFlow properties (for example _debugState,
_crossfadeEnabled) updated by the service and expose only the read-only
StateFlow/Flow/LiveData (debugState, crossfadeEnabled) so consumers can observe
state without mutating lifecycles or instances; replace internal declarations of
LoudnessEnhancer/DiscordRPC/ScrobbleManager with private instances and add
lightweight read-only references in the exposed DebugState/StateFlow to
represent their statuses instead of exposing the objects themselves.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt`:
- Around line 108-114: The PreferenceEntry currently throws a RuntimeException
directly in its onClick (the “Developer Triggered Crash” path); replace this
with a confirmation dialog flow: add a composable state (e.g., showConfirmCrash
: MutableState<Boolean>) in DevToolsSettingsScreen, change the PreferenceEntry
onClick to set showConfirmCrash = true, and render a Material3 AlertDialog when
showConfirmCrash is true with Cancel (sets showConfirmCrash = false) and Confirm
(throws the same RuntimeException) actions; keep the existing message/title
(stringResource R.string.test_crash_handler / R.string.test_crash_handler_desc)
and the exception text unchanged so the crash behavior remains identical after
confirmation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/kotlin/com/metrolist/music/App.ktapp/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.ktapp/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.ktapp/src/main/res/values/metrolist_strings.xmldevelopment_guide.md
🚧 Files skipped from review as they are similar to previous changes (1)
- development_guide.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: Quick Universal Release Build
- GitHub Check: Build Release (gms)
- GitHub Check: Build Debug (gms)
- GitHub Check: Build Release (foss)
- GitHub Check: Build Debug (foss)
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (2)
app/src/main/kotlin/com/metrolist/music/ui/component/IconButton.kt (1)
IconButton(62-95)app/src/main/kotlin/com/metrolist/music/ui/component/Preference.kt (2)
SwitchPreference(183-217)PreferenceEntry(48-105)
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
app/src/main/kotlin/com/metrolist/music/ui/component/IconButton.kt (1)
IconButton(62-95)
🔇 Additional comments (9)
app/src/main/kotlin/com/metrolist/music/App.kt (2)
62-64: LGTM on the DI injection setup.Clean Hilt injection pattern for the
DevToolsTimberTree. The singleton scope fromAppModuleensures a single instance across the app lifecycle.
74-82: Solid gating logic for the Timber trees — addresses the prior security concern.The implementation now properly:
- Gates
Timber.DebugTree()behindBuildConfig.DEBUG- Gates
DevToolsTimberTreebehind theDeveloperModeKeypreferenceOne behavioral note: since the tree is planted once during
onCreate(), toggling developer mode at runtime won't take effect until the app restarts. This is likely intentional (and simpler than dynamic tree management), but might be worth a quick tooltip or toast in the settings UI if users report confusion.The
runBlockingcall on line 79 is a brief blocking read — acceptable here given the existing pattern at line 237 forMaxImageCacheSizeKeyand the fact that DataStore local reads are typically sub-millisecond.app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
3128-3134: Nice teardown hardening inonDestroy.This null-before-close handoff with guarded failure logging is a solid lifecycle cleanup improvement.
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
429-438: Nice accessibility correction on the selection control.
IconButtonnow uses a 48.dp touch target, which aligns well with expected accessibility sizing while keeping the icon compact.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
317-344: Scaffold + insets wiring is clean.AI-agent check: this top bar/snackbar host setup is solid, and navigation behavior is preserved.
app/src/main/res/values/metrolist_strings.xml (1)
920-923: Plural handling for remaining taps is correctly implemented.Nice fix: quantity-aware wording now handles singular/plural correctly for the countdown UX.
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (1)
100-104: Disable flow is now UX-safe.AI-agent check: navigating up immediately when
devModeis turned off avoids the abrupt “screen disappears” state loop.app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (2)
43-58: Playback-speed normalization and time math look solid.Lines 43-58 handle non-positive speed safely and keep remaining time non-negative; this reduces bad RPC timestamps under edge inputs.
116-123:maskTokenbehavior now matches the docstring.Nice alignment on “first 4 / last 2” plus a short-token guard at Line 121.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
108-110:⚠️ Potential issue | 🟠 Major
close()likely still misses full socket teardown.This appears to be the same unresolved teardown concern from earlier review context: if
super.close()only clears activity, the websocket may remain open unlesscloseRPC()is also invoked.#!/bin/bash set -euo pipefail echo "Locating KizzyRPC source..." fd -t f "KizzyRPC.kt" echo echo "Inspecting close-related APIs in KizzyRPC..." rg -nP --type=kt 'override\s+suspend\s+fun\s+close\s*\(|suspend\s+fun\s+close\s*\(|fun\s+closeRPC\s*\(' kizzy/src/main/kotlin/com/my/kizzy/rpc/KizzyRPC.kt -C3 echo echo "Inspecting DiscordRPC close override..." rg -nP --type=kt 'override\s+suspend\s+fun\s+close\s*\(' app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt -C5Suggested minimal fix
override suspend fun close() { Timber.d("DiscordRPC closing connection") super.close() + closeRPC() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt` around lines 108 - 110, The override suspend fun close() in DiscordRPC currently calls super.close() but likely omits full socket teardown; update DiscordRPC.close() to also call the RPC socket shutdown method (e.g., closeRPC() or the equivalent in KizzyRPC) after or before super.close() to ensure the websocket is closed and resources released; locate the DiscordRPC class and its override of close(), and invoke the existing closeRPC() (or the concrete socket-close method in KizzyRPC) so both activity cleanup (super.close()) and socket teardown occur.
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
44-45: Consider throttling song metadata debug logs inupdateSong.This path can execute frequently; logging title/artist every call can quickly churn the circular buffer and bury higher-value diagnostics. Recommend logging only on track change (or sampling).
Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt` around lines 44 - 45, The debug logging in updateSong (the Timber.d calls) runs every invocation and should be throttled: change the logic so you only emit these detailed title/artist logs when the track actually changes (e.g., compare current song.song.title and artists.joinToString to a stored lastLogged identifier or timestamp) or implement simple sampling/rate-limiter before calling Timber.d; update both occurrences of the Timber.d in updateSong to use this guard and update the stored last-logged song state accordingly.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
391-393: Consider adding error handling to DataStore write for robustness, though this follows the codebase's established pattern.The
dataStore.edit()call at line 392 lacks error handling. While the codebase consistently uses this pattern throughout (including in the DataStore utility wrapper itself), adding a try/catch would improve defensiveness against storage failures. If a write fails, the coroutine completes silently without user feedback.🔧 Suggested patch
remaining <= 0 -> { tapCount = 0 lastTapTime = 0L coroutineScope.launch { - context.dataStore.edit { it[DeveloperModeKey] = true } - Toast.makeText(context, context.getString(R.string.dev_mode_enabled), Toast.LENGTH_LONG).show() + try { + context.dataStore.edit { it[DeveloperModeKey] = true } + Toast.makeText(context, context.getString(R.string.dev_mode_enabled), Toast.LENGTH_LONG).show() + } catch (e: Exception) { + Toast.makeText(context, "Failed to enable developer mode", Toast.LENGTH_SHORT).show() + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt` around lines 391 - 393, Wrap the DataStore write inside coroutineScope.launch with a try/catch around context.dataStore.edit to handle failures: call context.dataStore.edit { it[DeveloperModeKey] = true } inside the try, show the existing success Toast via Toast.makeText(...) there, and in the catch log the exception and show a failure Toast (or fallback message) so users get feedback if the write fails; ensure you reference coroutineScope.launch, context.dataStore.edit, DeveloperModeKey and Toast.makeText when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 108-110: The override suspend fun close() in DiscordRPC currently
calls super.close() but likely omits full socket teardown; update
DiscordRPC.close() to also call the RPC socket shutdown method (e.g., closeRPC()
or the equivalent in KizzyRPC) after or before super.close() to ensure the
websocket is closed and resources released; locate the DiscordRPC class and its
override of close(), and invoke the existing closeRPC() (or the concrete
socket-close method in KizzyRPC) so both activity cleanup (super.close()) and
socket teardown occur.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`:
- Around line 391-393: Wrap the DataStore write inside coroutineScope.launch
with a try/catch around context.dataStore.edit to handle failures: call
context.dataStore.edit { it[DeveloperModeKey] = true } inside the try, show the
existing success Toast via Toast.makeText(...) there, and in the catch log the
exception and show a failure Toast (or fallback message) so users get feedback
if the write fails; ensure you reference coroutineScope.launch,
context.dataStore.edit, DeveloperModeKey and Toast.makeText when making the
changes.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 44-45: The debug logging in updateSong (the Timber.d calls) runs
every invocation and should be throttled: change the logic so you only emit
these detailed title/artist logs when the track actually changes (e.g., compare
current song.song.title and artists.joinToString to a stored lastLogged
identifier or timestamp) or implement simple sampling/rate-limiter before
calling Timber.d; update both occurrences of the Timber.d in updateSong to use
this guard and update the stored last-logged song state accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.ktapp/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: Build Release (foss)
- GitHub Check: Build Release (gms)
- GitHub Check: Build Debug (gms)
- GitHub Check: Build Debug (foss)
- GitHub Check: Quick Universal Release Build
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
app/src/main/kotlin/com/metrolist/music/ui/component/IconButton.kt (1)
IconButton(62-95)
🔇 Additional comments (5)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (3)
26-28: Good secret-handling hardening in init log.AI-agent check passes: this no longer emits token-derived content, which is the right security posture for auth material.
43-58: Playback speed guard + time math update looks solid.
validPlaybackSpeedprevents divide-by-zero/negative-speed issues, and the adjusted start/end timing flow is consistent.
116-123:maskTokendocs and implementation are now aligned.Line 121 short-token guard + Line 122
4...2masking format are consistent with the KDoc.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
388-390: Nice fix: synchronous reset closes rapid-tap duplicate activation window.Agent check: resetting state before async work is the right move here and resolves the earlier race pattern cleanly.
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
86-87: Use stable tag-group IDs instead of localized labels.AI-agent heads-up:
selectedTagGroupsis currently keyed by translated strings. If locale/translation changes, selection and matching can desync and filters may silently stop matching. Keep stable internal keys (enum/sealed class) in state, then map keys tostringResourceonly for chip labels.♻️ Minimal refactor sketch
+private enum class TagGroup { PLAYER, UI, DB, INTEGRATION } - var selectedTagGroups by remember { mutableStateOf(setOf<String>()) } + var selectedTagGroups by remember { mutableStateOf(setOf<TagGroup>()) } - val tagPlayer = stringResource(R.string.dev_filter_player) - val tagUi = stringResource(R.string.dev_filter_ui) - val tagDb = stringResource(R.string.dev_filter_db) - val tagIntegration = stringResource(R.string.dev_filter_integration) + val tagGroups = listOf( + TagGroup.PLAYER to stringResource(R.string.dev_filter_player), + TagGroup.UI to stringResource(R.string.dev_filter_ui), + TagGroup.DB to stringResource(R.string.dev_filter_db), + TagGroup.INTEGRATION to stringResource(R.string.dev_filter_integration), + ) - when (group) { - tagPlayer -> ... - tagUi -> ... - tagDb -> ... - tagIntegration -> ... - else -> false - } + when (group) { + TagGroup.PLAYER -> ... + TagGroup.UI -> ... + TagGroup.DB -> ... + TagGroup.INTEGRATION -> ... + }Also applies to: 104-108, 117-124, 283-294
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`:
- Around line 392-397: In AboutScreen.kt update the DataStore edit error
handling so CancellationException is rethrown (preserve coroutine cancellation)
and all other exceptions show a localized message: catch CancellationException
and throw it, then catch Exception and use
context.getString(R.string.dev_mode_enable_failed) when showing the Toast; also
add the string resource name dev_mode_enable_failed to your strings.xml with the
appropriate message. Ensure the try block around context.dataStore.edit and the
Toast on success remain unchanged and reference the same symbols
(DeveloperModeKey, context.dataStore.edit) so the change is localized.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Line 110: Throttle the noisy success log by only emitting
Timber.d("updateSong: activity set successfully...") on actual track transitions
(same gating used earlier in updateSong) instead of every tick: find the
updateSong method in DiscordRPC.kt and move the Timber.d call inside the
existing new-song conditional (compare current song id/title to the previously
tracked song variable like previousSong/lastSong or lastLoggedSongId), or add a
small field (e.g., lastLoggedSongId) to store the last-logged track and update
it when the track changes so the success message is logged only on song
transitions.
- Around line 113-116: The close ordering is wrong: call super.close() before
closing the socket so the clear-activity payload sent by KizzyRPC.close() (which
uses discordWebSocket.sendActivity()) can be transmitted; change the
implementation of close() to invoke super.close() first, then call closeRPC()
(which calls discordWebSocket.close()) so the empty Presence is sent
successfully prior to socket teardown.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.ktapp/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: Build Debug (gms)
- GitHub Check: Build Release (foss)
- GitHub Check: Build Release (gms)
- GitHub Check: Build Debug (foss)
- GitHub Check: Quick Universal Release Build
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
kizzy/src/main/kotlin/com/my/kizzy/rpc/KizzyRPC.kt (1)
closeRPC(41-43)
🪛 detekt (1.23.8)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt
[warning] 395-395: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (5)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (2)
45-63: Nice stabilization on playback-speed handling and timestamp math.Line 45 and Lines 53-63 are robust: invalid speed fallback, non-negative remaining duration, and speed-adjusted timestamps are correctly handled.
122-129:maskTokendoc and implementation are aligned now.Line 123-124 and Line 128 are consistent (4 leading + 2 trailing chars), and Line 127 properly guards short tokens.
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (3)
95-97: Nice parameterization of the dev-mode tap rules.Extracting tap thresholds/timeouts into constants makes the easter-egg logic easier to tune and safer to maintain.
317-344: Scaffold integration is clean and well-wired.Top bar + snackbar host placement (with player-aware insets) looks correct and keeps the screen structure modular.
389-391: Good guard against duplicate activations in the same tap burst.Resetting state before launching the suspend write closes the rapid-tap window that existed earlier.
| try { | ||
| context.dataStore.edit { it[DeveloperModeKey] = true } | ||
| Toast.makeText(context, context.getString(R.string.dev_mode_enabled), Toast.LENGTH_LONG).show() | ||
| } catch (e: Exception) { | ||
| Toast.makeText(context, "Failed to enable developer mode", Toast.LENGTH_SHORT).show() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the AboutScreen.kt file
find . -name "AboutScreen.kt" -type fRepository: adrielGGmotion/Metrolist
Length of output: 144
🏁 Script executed:
# Get file size and examine the relevant lines
if [ -f "app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt" ]; then
wc -l "app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt"
echo "=== Lines 385-405 ==="
sed -n '385,405p' "app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt"
fiRepository: adrielGGmotion/Metrolist
Length of output: 1615
🏁 Script executed:
# Search for the error resource string mentioned in the patch
find . -name "strings.xml" -type f | head -20Repository: adrielGGmotion/Metrolist
Length of output: 907
🏁 Script executed:
# Check for Timber imports in the project
rg "import timber" --type kotlin | head -10Repository: adrielGGmotion/Metrolist
Length of output: 941
🏁 Script executed:
# Check if dev_mode_enable_failed exists in strings resources
rg "dev_mode_enable_failed" . --type xmlRepository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Look for the context and verify if this is inside a coroutine launch
sed -n '370,410p' "app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt"Repository: adrielGGmotion/Metrolist
Length of output: 2702
🏁 Script executed:
# Search for dev_mode related strings
rg "dev_mode" app/src/main/res/values/strings.xml --context 2Repository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Check what other dev mode strings exist
rg "dev_mode" app/src/main/res/values/ -t xml | head -20Repository: adrielGGmotion/Metrolist
Length of output: 547
🏁 Script executed:
# Check the AboutScreen.kt file for imports to see if Timber is already imported
sed -n '1,50p' "app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt" | grep -E "^import|^package"Repository: adrielGGmotion/Metrolist
Length of output: 2258
Preserve coroutine cancellation and localize error messaging.
The catch (e: Exception) block at line 395 catches CancellationException, suppressing cancellation semantics. Additionally, the hardcoded error string "Failed to enable developer mode" is non-localized—there's no corresponding dev_mode_enable_failed resource in your strings files.
The fix needs two parts:
- Explicitly handle
CancellationExceptionseparately to preserve coroutine cancellation - Either create a new string resource
dev_mode_enable_failedor reuse an existing pattern from your codebase
Here's the corrected structure:
🔧 Suggested approach
- } catch (e: Exception) {
- Toast.makeText(context, "Failed to enable developer mode", Toast.LENGTH_SHORT).show()
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
+ Toast.makeText(
+ context,
+ context.getString(R.string.dev_mode_enable_failed),
+ Toast.LENGTH_SHORT
+ ).show()
}Then add the missing resource string to your strings.xml:
<string name="dev_mode_enable_failed">Failed to enable developer mode</string>🧰 Tools
🪛 detekt (1.23.8)
[warning] 395-395: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`
around lines 392 - 397, In AboutScreen.kt update the DataStore edit error
handling so CancellationException is rethrown (preserve coroutine cancellation)
and all other exceptions show a localized message: catch CancellationException
and throw it, then catch Exception and use
context.getString(R.string.dev_mode_enable_failed) when showing the Toast; also
add the string resource name dev_mode_enable_failed to your strings.xml with the
appropriate message. Ensure the try block around context.dataStore.edit and the
Toast on success remain unchanged and reference the same symbols
(DeveloperModeKey, context.dataStore.edit) so the change is localized.
| applicationId = APPLICATION_ID, | ||
| status = status | ||
| ) | ||
| Timber.d("updateSong: activity set successfully for \"%s\"", song.song.title) |
There was a problem hiding this comment.
Throttle success logging too; Line 110 still logs on every update tick.
Line 110 can generate noisy high-frequency logs and churn the in-memory DevTools buffer. Keep this log gated to new-song transitions like the earlier debug block.
Proposed throttle-aligned diff
- val validPlaybackSpeed = if (playbackSpeed <= 0f) 1.0f else playbackSpeed
- if (song.song.id != lastLoggedSongId) {
+ val validPlaybackSpeed = if (playbackSpeed <= 0f) 1.0f else playbackSpeed
+ val isNewSong = song.song.id != lastLoggedSongId
+ if (isNewSong) {
lastLoggedSongId = song.song.id
Timber.d("updateSong: title=\"%s\", artist=\"%s\", activityType=%s",
song.song.title, song.artists.joinToString { it.name }, activityType)
}
@@
- Timber.d("updateSong: activity set successfully for \"%s\"", song.song.title)
+ if (isNewSong) {
+ Timber.d("updateSong: activity set successfully for \"%s\"", song.song.title)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt` at line 110,
Throttle the noisy success log by only emitting Timber.d("updateSong: activity
set successfully...") on actual track transitions (same gating used earlier in
updateSong) instead of every tick: find the updateSong method in DiscordRPC.kt
and move the Timber.d call inside the existing new-song conditional (compare
current song id/title to the previously tracked song variable like
previousSong/lastSong or lastLoggedSongId), or add a small field (e.g.,
lastLoggedSongId) to store the last-logged track and update it when the track
changes so the success message is logged only on song transitions.
| override suspend fun close() { | ||
| Timber.d("DiscordRPC closing connection") | ||
| closeRPC() | ||
| super.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate KizzyRPC implementation =="
find . -type f -name "KizzyRPC.kt"
echo -e "\n== Locate DiscordRPC implementation =="
find . -type f -name "DiscordRPC.kt"Repository: adrielGGmotion/Metrolist
Length of output: 259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KizzyRPC.kt - check file size and find close methods =="
wc -l ./kizzy/src/main/kotlin/com/my/kizzy/rpc/KizzyRPC.kt
echo -e "\n== Search for close-related methods in KizzyRPC =="
rg -n "fun close|suspend fun close|closeRPC" ./kizzy/src/main/kotlin/com/my/kizzy/rpc/KizzyRPC.kt
echo -e "\n== DiscordRPC.kt - check close() override context =="
sed -n '100,130p' ./app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.ktRepository: adrielGGmotion/Metrolist
Length of output: 1497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KizzyRPC closeRPC() implementation =="
sed -n '40,55p' ./kizzy/src/main/kotlin/com/my/kizzy/rpc/KizzyRPC.kt
echo -e "\n== Full KizzyRPC.kt to understand close() behavior =="
cat -n ./kizzy/src/main/kotlin/com/my/kizzy/rpc/KizzyRPC.kt | sed -n '40,80p'
echo -e "\n== Check DiscordRPC closeRPC() implementation =="
rg -n -A 10 "fun closeRPC" ./app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.ktRepository: adrielGGmotion/Metrolist
Length of output: 1998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if DiscordRPC defines closeRPC or inherits from parent =="
grep -n "fun closeRPC\|open suspend fun close\|override suspend fun close" ./app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt
echo -e "\n== Check DiscordRPC class declaration and parent =="
head -30 ./app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.ktRepository: adrielGGmotion/Metrolist
Length of output: 967
Reverse close() order — super.close() must run before closeRPC().
The current implementation closes the socket first via closeRPC() (which calls discordWebSocket.close()), then super.close() attempts to send a clear-activity payload. Since the socket is already closed, that payload never reaches Discord's servers, leaving the activity stuck as "active" even after the app closes.
KizzyRPC.close() sends discordWebSocket.sendActivity() with an empty Presence to clear the activity — this must complete before the socket closes. Call super.close() before closeRPC().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt` around lines 113
- 116, The close ordering is wrong: call super.close() before closing the socket
so the clear-activity payload sent by KizzyRPC.close() (which uses
discordWebSocket.sendActivity()) can be transmitted; change the implementation
of close() to invoke super.close() first, then call closeRPC() (which calls
discordWebSocket.close()) so the empty Presence is sent successfully prior to
socket teardown.
|
@coderabbitai Verify if there are any other nipticks or necessary changes to be made. |
|
@gemini-code-assist Verify if there are any other nipticks or necessary changes to be made. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 11720 🏁 Script executed: Length of output: 3775 🏁 Script executed: Length of output: 941 🏁 Script executed: Length of output: 43 🏁 Script executed: Length of output: 40 🏁 Script executed: Length of output: 1714 🏁 Script executed: Length of output: 169 🏁 Script executed: Length of output: 48 🏁 Script executed: Length of output: 3827 🏁 Script executed: Length of output: 43 🏁 Script executed: Length of output: 41 🏁 Script executed: Length of output: 547 🏁 Script executed: Length of output: 427 🏁 Script executed: Length of output: 2204 🏁 Script executed: Length of output: 1193 🏁 Script executed: Length of output: 3424 🏁 Script executed: Length of output: 292 🏁 Script executed: Length of output: 535 🏁 Script executed: Length of output: 693 🏁 Script executed: Length of output: 43 🏁 Script executed: Length of output: 738 🏁 Script executed: Length of output: 1314 🏁 Script executed: Length of output: 1036 🏁 Script executed: Length of output: 78 🏁 Script executed: Length of output: 157 🏁 Script executed: Length of output: 38 🏁 Script executed: Length of output: 1430 🏁 Script executed: Length of output: 987 🏁 Script executed: Length of output: 455 🏁 Script executed: Length of output: 2670
🔴 Critical IssueApp.kt - runBlocking on Main Thread (Lines 79 & 237) The app uses // Line 79 in onCreate()
val devModeEnabled = runBlocking { dataStore.data.first()[DeveloperModeKey] } ?: false
// Line 237 in newImageLoader()
val cacheSize = runBlocking {
dataStore.data.map { it[MaxImageCacheSizeKey] ?: 512 }.first()
}Risk: These blocking calls can cause ANR (Application Not Responding) if DataStore reads are slow, especially on cold app starts or older devices. Recommendation: // Option 1: Make devToolsTimberTree lazy
private val devToolsTimberTree by lazy {
DevToolsTimberTree(devToolsLogBuffer).also { tree ->
applicationScope.launch {
if (dataStore.data.first()[DeveloperModeKey] == true) {
Timber.plant(tree)
}
}
}
}
// Option 2: Plant asynchronously in applicationScope
override fun onCreate() {
super.onCreate()
// ... other initialization ...
applicationScope.launch {
val devModeEnabled = dataStore.data.first()[DeveloperModeKey] ?: false
if (devModeEnabled) {
Timber.plant(devToolsTimberTree)
}
}
}✅ What's Working Well
📝 Minor Suggestions
SummaryThe implementation is solid overall with good architecture and error handling. The only critical issue is the |
|
I'll commit sui*ide if there is another error |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-structured set of developer tools, which is a fantastic addition for debugging the application. The implementation includes a floating overlay, in-memory logging, and UI panels for inspecting logs, player state, and the database. The code is generally of high quality. I've identified one critical performance issue on app startup and a minor inaccuracy in the cache clearing logic. My detailed feedback is in the comments below.
| val devModeEnabled = runBlocking { dataStore.data.first()[DeveloperModeKey] } ?: false | ||
| if (devModeEnabled) { | ||
| Timber.plant(devToolsTimberTree) | ||
| } |
There was a problem hiding this comment.
Using runBlocking on the main thread during Application.onCreate() is a significant performance concern and can lead to Application Not Responding (ANR) errors, as it blocks the UI thread for disk I/O. A better approach is to asynchronously observe the preference and plant or uproot the DevToolsTimberTree when the value changes. This can be done within the observeSettingsChanges method.
// This block should be removed from here.
| val sizeMb = withContext(Dispatchers.IO) { | ||
| val size = context.cacheDir.walkTopDown().filter { it.isFile }.map { it.length() }.sum() | ||
| val files = context.cacheDir.listFiles() | ||
| var allDeleted = true | ||
| files?.forEach { if (!it.deleteRecursively()) allDeleted = false } | ||
| if (!allDeleted) { | ||
| // Some files couldn't be deleted, but we continue | ||
| } | ||
| size / 1024 / 1024 | ||
| } |
There was a problem hiding this comment.
The size of the cleared cache is calculated before the deletion occurs. If some files fail to be deleted, the reported size will be inaccurate. Additionally, using integer division (size / 1024 / 1024) will truncate the result (e.g., 1.9 MB will be reported as 1 MB).
Consider calculating the size of successfully deleted files for a more accurate report, or simply show a success message without the size.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/App.kt (1)
245-250: Prefer bounded dynamic disk-cache sizing over fixed 512MB.A fixed 512MB cache can be too heavy on constrained devices. Use a % of available storage with min/max caps to reduce storage-pressure risk.
Proposed refactor
diskCache( DiskCache.Builder() .directory(cacheDir.resolve("coil")) - .maxSizeBytes(512 * 1024 * 1024L) + .maxSizeBytes( + (cacheDir.usableSpace * 0.02).toLong() + .coerceIn( + 64L * 1024 * 1024, // 64MB floor + 512L * 1024 * 1024 // 512MB ceiling + ) + ) .build() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/App.kt` around lines 245 - 250, Replace the fixed 512MB disk cache size with a bounded dynamic size: compute available storage from cacheDir (e.g., via StatFs on cacheDir.path), derive a percentage of total storage (for example 3–5%), clamp that value between sensible min and max caps (e.g., 64 * 1024 * 1024L and 512 * 1024 * 1024L), and pass the resulting Long into DiskCache.Builder().maxSizeBytes(...) instead of the hardcoded 512 * 1024 * 1024L; update the diskCache block that calls DiskCache.Builder(), and ensure the computed value is used when building the cache.
🤖 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/kotlin/com/metrolist/music/App.kt`:
- Around line 79-86: The DataStore read (dataStore.data.first()) inside the
applicationScope.launch can throw and prevent initializeSettings() and
observeSettingsChanges() from running; wrap that read in a guarded operation
(try/catch or runCatching) around the call that reads DeveloperModeKey so
failures default devModeEnabled to false and do not rethrow, then conditionally
call Timber.plant(devToolsTimberTree) only if devModeEnabled is true; ensure
initializeSettings() and observeSettingsChanges() are executed unconditionally
outside the failing read (i.e., move them after the guarded read or into a
finally block) so core startup always runs.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/App.kt`:
- Around line 245-250: Replace the fixed 512MB disk cache size with a bounded
dynamic size: compute available storage from cacheDir (e.g., via StatFs on
cacheDir.path), derive a percentage of total storage (for example 3–5%), clamp
that value between sensible min and max caps (e.g., 64 * 1024 * 1024L and 512 *
1024 * 1024L), and pass the resulting Long into
DiskCache.Builder().maxSizeBytes(...) instead of the hardcoded 512 * 1024 *
1024L; update the diskCache block that calls DiskCache.Builder(), and ensure the
computed value is used when building the cache.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/App.kt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: Build Debug (foss)
- GitHub Check: Build Release (gms)
- GitHub Check: Build Release (foss)
- GitHub Check: Build Debug (gms)
- GitHub Check: Quick Universal Release Build
🔇 Additional comments (1)
app/src/main/kotlin/com/metrolist/music/App.kt (1)
74-76: Good guard for debug-only Logcat tree.
Timber.DebugTree()is now correctly scoped to debug builds, which avoids unnecessary release logging noise.
|
@coderabbitai full check |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19713
✅ Critical Issue RESOLVEDYour commit successfully addressed the ANR risk: App.kt Changes:
applicationScope.launch {
val devModeEnabled = runCatching {
dataStore.data.first()[DeveloperModeKey] ?: false
}.getOrElse {
Timber.w(it, "Failed to read DeveloperModeKey; continuing startup")
false
}
if (devModeEnabled) {
Timber.plant(devToolsTimberTree)
}
// ... rest of async initialization
}✅ Full DevTools Implementation CheckCode Quality:
Architecture:
Other 🚀 VerdictNo errors detected. Your DevTools implementation is solid and production-ready. The async initialization pattern you implemented is exactly right, and all the DevTools code follows Android best practices. Ship it! 🎯 |
…rol panels
This commit introduces a comprehensive Developer Tools suite to Metrolist, providing a floating overlay (FAB) accessible when developer mode is enabled.
Key features include:
- Floating DevTools Overlay: A draggable FAB that provides quick access to diagnostic panels without leaving the current screen.
- Real-time Log Viewer: Integrated Timber tree that captures app logs into a circular buffer, viewable directly within the app with support for filtering and color-coded priority levels.
- Player State Diagnostics: Detailed view of the current playback state, including MediaController status, MusicService state, and active media metadata.
- Database Information: High-level overview of the local Room database, including track counts, playlist stats, and internal versioning.
- Developer Actions:
- Log Export: Generates a detailed diagnostic report including system environment details (device, OS, RAM, resolution) and redacted logs.
- Data Redaction: Automatic stripping of sensitive patterns (auth tokens, cookies, visitor data) from exported logs to ensure user privacy.
- Cache Management: Direct actions to clear application cache and Coil image cache for debugging image loading and storage issues.
- Settings Integration: A new Developer Tools section in settings to toggle the overlay and configure related options.
The implementation follows Material 3 design guidelines, utilizing expressive components like WavyProgressIndicator and modern TabRow layouts.
8a89edd to
62c1d7f
Compare
|
@coderabbitai I've fixed one commit and squashed all the commits history into a single one. Ensure there are no more issues and mergeability in a scale of 1-10 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6523
🎯 Mergeability Rating: 9/10✅ What's PerfectCritical Issues (100% resolved)
Architecture & Integration (Flawless)
Code Quality
📝 Minor Gap (-1 point)Testing Coverage
🚀 Merge Confidence
Verdict: This PR is production-ready and safe to merge. The DevTools implementation follows Android best practices, introduces zero risk to existing functionality, and adds significant value for debugging. The single point deduction is for missing concurrent-access unit tests, which is acceptable for an internal debugging tool. Ship it with confidence! 🎉 |
2 participants
Problem
Metrolist lacked developer tools for debugging runtime issues. When problems occurred (e.g., playback failures, sync issues), users and developers had no way to inspect the app's internal state, view logs, or access database information for troubleshooting.
Cause
The app didn't have any built-in debugging utilities. Logs were only visible in Logcat (requiring ADB), and there was no way to inspect playback state, database contents, or trigger diagnostic actions without external tools.
Solution
DevToolsTimberTreeto capture logs in-memoryMusicService(crossfade, Discord RPC, scrobbling, loudness enhancer) asinternalTesting
./gradlew :app:assembleuniversalFossDebugRelated Issues
NoneNoneSummary by CodeRabbit
New Features
Chores