This repository was archived by the owner on Apr 10, 2026. It is now read-only.
feat(devtools): add developer tools overlay with log viewer, player s… - #50
Closed
adrielGGmotion wants to merge 3 commits into
Closed
feat(devtools): add developer tools overlay with log viewer, player s…#50adrielGGmotion wants to merge 3 commits into
adrielGGmotion wants to merge 3 commits into
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 DevTools feature: captures Timber logs into a ring buffer, exposes a draggable Compose overlay (Logs, Player, DB, Tools), new actions and DB panels, a settings toggle and easter-egg, DI providers and a Timber tree wired into app startup, plus related strings, resources, and expanded logging across services. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (startup)
participant Timber as Timber
participant DevTree as DevToolsTimberTree
participant Buffer as DevToolsLogBuffer
participant UI as DevToolsOverlay / LogViewer
App->>Timber: plant DebugTree (if DEBUG)
App->>Timber: plant DevToolsTimberTree
Note right of Timber: app/global logging calls
Timber->>DevTree: log(priority, tag, message, t)
DevTree->>Buffer: add(DevToolsLog)
Buffer->>UI: emit logs StateFlow
UI->>Buffer: clear() / read logs / export
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 developer tools overlay to aid in debugging and inspecting the application's internal state. It provides real-time logging, playback information, database statistics, and convenient actions for cache management and log exporting. The overlay is accessible only when developer mode is enabled in settings. Highlights
Changelog
Activity
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
30-33:⚠️ Potential issue | 🟠 MajorValidate
playbackSpeedbefore timestamp calculations.Line 47 and Line 57 assume a strictly positive speed. If speed is
<= 0f, presence timestamps become invalid and can corrupt activity timing.✅ Proposed guard
) = runCatching { + require(playbackSpeed > 0f) { "playbackSpeed must be > 0" } + Timber.d("updateSong: title=\"%s\", artist=\"%s\", activityType=%s", song.song.title, song.artists.joinToString { it.name }, activityType)Also applies to: 47-48, 56-58
🤖 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 30 - 33, In updateSong, validate playbackSpeed before any timestamp math: ensure playbackSpeed > 0 (or default to 1.0f) and handle non-positive values early to avoid dividing by/using zero or negative speed when computing durationMillis, startTimestamp and endTimestamp; update the logic around currentPlaybackTimeMillis, durationMillis and the presence start/end timestamp calculations to use the validated/clamped playbackSpeed (or bail out/skip presence update) so timestamps remain valid; reference function updateSong, parameters currentPlaybackTimeMillis and playbackSpeed, and the presence timestamp computations (startTimestamp/endTimestamp) when making the change.
🧹 Nitpick comments (8)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
27-28: Consider removing song metadata from DiscordRPC debug logs.Lines 43-44 and 104 log song titles and artist names. While Timber trees are properly gated to DEBUG builds only (App.kt lines 74-76), reducing logged metadata improves privacy in development environments.
The masked token approach at line 27 is acceptable.
Suggested refinement
- Timber.d("updateSong: title=\"%s\", artist=\"%s\", activityType=%s", - song.song.title, song.artists.joinToString { it.name }, activityType) + Timber.d("updateSong: activityType=%s, hasButton1=%s, hasButton2=%s", + activityType, button1Visible, button2Visible)- Timber.d("updateSong: activity set successfully for \"%s\"", song.song.title) + Timber.d("updateSong: activity set successfully")Also applies to: 43-44, 104
🤖 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 27 - 28, The debug logs in DiscordRPC are emitting song metadata; update the Timber.d calls in the DiscordRPC class (remove or redact song title/artist fields) so they no longer log plaintext song metadata—e.g., in the methods that currently log track info (the Timber.d calls that reference song title/artist, such as the presence/update handlers or track-change methods), replace the logged title/artist with a redacted placeholder or log only non-sensitive identifiers (e.g., a track ID or boolean flags) while keeping the existing masked token log intact.app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt (1)
36-36: Extract the repeated"LastFM"tag into one constant.This reduces typo risk and simplifies future tag updates.
♻️ Suggested refactor
class ScrobbleManager( @@ ) { + private companion object { + const val LASTFM_TAG = "LastFM" + } @@ - Timber.tag("LastFM").d("ScrobbleManager destroyed") + Timber.tag(LASTFM_TAG).d("ScrobbleManager destroyed")Also applies to: 43-43, 51-51, 56-56, 61-61, 71-71, 83-83, 98-98, 106-106, 118-118, 122-122, 135-135
🤖 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/ScrobbleManager.kt` at line 36, Multiple Timber.tag("LastFM") usages in ScrobbleManager are repeated string literals; add a single constant (e.g., private const val TAG = "LastFM") in ScrobbleManager (companion object or top-level) and update all calls like Timber.tag("LastFM").d(...) / Timber.tag("LastFM").i(...) etc. to use Timber.tag(TAG) so the tag is centralized and avoids typos (affects occurrences referenced in ScrobbleManager).app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt (1)
431-442: Extract day-time parsing into a shared non-UI utility.This parser duplicates serialization logic already present in
SleepTimerDialog.kt. Moving it to a shared utility reduces drift between UI persistence and playback evaluation.🤖 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/PlayerConnection.kt` around lines 431 - 442, The parseDayTimes function duplicates logic in SleepTimerDialog; extract parseDayTimes into a shared non-UI utility (e.g., a new object or util file) and have both PlayerConnection.parseDayTimes and SleepTimerDialog use that single function; move the logic currently in private fun parseDayTimes(raw: String): Map<Int, Pair<String, String>> into the new utility (preserving signature or providing a compatible public function), replace the private function in PlayerConnection with a call to the shared util, and update SleepTimerDialog to call the same util so serialization/parsing logic is centralized and reused.app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt (2)
52-58: Consider usingdimensionResourcefor cleaner code.The manual dimension retrieval via
context.resources.getDimension()with density conversion can be simplified using Compose'sdimensionResource.♻️ Proposed simplification
+import androidx.compose.ui.res.dimensionResource `@Composable` private fun DevToolsFabBottomPadding(): androidx.compose.ui.unit.Dp { - val context = androidx.compose.ui.platform.LocalContext.current - val density = androidx.compose.ui.platform.LocalDensity.current - val value = context.resources.getDimension(com.metrolist.music.R.dimen.devtools_fab_bottom_padding) - return with(density) { value.toDp() } + return dimensionResource(R.dimen.devtools_fab_bottom_padding) }🤖 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 52 - 58, DevToolsFabBottomPadding manually fetches a dimension and converts it to Dp; replace that logic with Compose's dimensionResource to simplify and remove manual density conversion: inside DevToolsFabBottomPadding use androidx.compose.ui.res.dimensionResource(R.dimen.devtools_fab_bottom_padding) (or import dimensionResource) and return it as Dp, removing LocalContext/LocalDensity and the with(density){...} conversion.
115-134: Potential UX issue: FAB drag bounds may be confusing.The FAB is aligned to
CenterEnd(right side, vertical center) but the drag offset starts at(0, 0). The bounds calculation allowsoffsetXto go negative (moving left) but not positive. This works, but if the user drags the FAB and then the screen rotates, the FAB position resets due torememberwithoutrememberSaveable.Consider using
rememberSaveableforoffsetXandoffsetYto persist position across configuration changes.♻️ Proposed fix for position persistence
- var offsetX by remember { mutableFloatStateOf(0f) } - var offsetY by remember { mutableFloatStateOf(0f) } + var offsetX by rememberSaveable { mutableFloatStateOf(0f) } + var offsetY by rememberSaveable { mutableFloatStateOf(0f) }Note: You'll need to add
import androidx.compose.runtime.saveable.rememberSaveable.🤖 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 115 - 134, The FAB's position (offsetX, offsetY) is only remembered in-memory and resets on configuration changes; update DevToolsOverlay to persist these values by replacing remember with rememberSaveable for offsetX and offsetY (and add the import androidx.compose.runtime.saveable.rememberSaveable), ensuring the drag logic in the FloatingActionButton still uses the same variables (offsetX, offsetY) and bounds checking so position survives rotations and process restarts.app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt (1)
19-20: Unnecessary@OptInannotations on both composables.Neither
InfoCardnorInfoRowuses any Material 3 Expressive APIs. These annotations add noise and may cause confusion about API stability requirements.♻️ Proposed fix
-@OptIn(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class) `@Composable` fun InfoCard(title: String, content: `@Composable` () -> Unit) {-@OptIn(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class) `@Composable` fun InfoRow(label: String, value: String, modifier: Modifier = Modifier) {Also applies to: 37-38
🤖 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/SharedDevToolsUI.kt` around lines 19 - 20, Remove the unnecessary `@OptIn`(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class) annotations applied to the composable declarations for InfoCard and InfoRow; locate the annotations immediately above the `@Composable` fun InfoCard(...) and `@Composable` fun InfoRow(...) and delete those `@OptIn` lines so the composables compile without opting into ExperimentalMaterial3ExpressiveApi.app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
376-380: Inconsistent click behavior may confuse users.When a log is selected, clicking it deselects. When not selected, clicking expands/collapses. This means users cannot expand a selected log without first deselecting it. Consider using long-press for selection toggle instead, or make the expand/collapse independent of selection state.
♻️ Suggestion: Use long-press for selection
+import androidx.compose.foundation.combinedClickable +import androidx.compose.foundation.ExperimentalFoundationApi Surface( color = if (isSelected) MaterialTheme.colorScheme.primaryContainer.copy(alpha = 0.3f) else MaterialTheme.colorScheme.surface, - modifier = Modifier.clickable { - if (isSelected) onToggleSelect(log.id) else expanded = !expanded - } + modifier = Modifier.combinedClickable( + onClick = { expanded = !expanded }, + onLongClick = { onToggleSelect(log.id) } + ) ) {This would also allow removing the selection toggle
IconButtonin the row header (lines 415-425) to reduce visual clutter.🤖 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 376 - 380, The Surface click currently toggles selection when isSelected and otherwise toggles expanded, which prevents expanding a selected log; update the interaction so expand/collapse is independent of selection and move selection to a long-press: replace the single Modifier.clickable usage in LogViewerPanel's Surface with a combinedClickable (or similar) that always toggles expanded (flip the expanded state) on normal click and calls onToggleSelect(log.id) onLongClick; also remove or hide the header IconButton selection toggle (the header IconButton referenced in the row header block) if you adopt long-press selection to avoid duplicate controls and visual clutter.app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt (1)
22-23: Unnecessary@OptInannotation.The
ExperimentalMaterial3ExpressiveApiopt-in appears unused in this composable. Consider removing it unlessPanelHeaderinternally requires it (in which case the opt-in should be at the call site of that function, not here).♻️ Proposed fix
-@OptIn(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class) `@Composable` fun DatabaseInfoPanel() {🤖 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 22 - 23, The `@OptIn`(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class) annotation on the DatabaseInfoPanel composable is unnecessary; remove that annotation from DatabaseInfoPanel unless the called composable PanelHeader actually requires the opt-in—if PanelHeader requires it, move the `@OptIn` to the call site of PanelHeader (or annotate PanelHeader itself) instead of annotating DatabaseInfoPanel, so only the functions that use ExperimentalMaterial3ExpressiveApi are opting in.
🤖 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/DevToolsLogBuffer.kt`:
- Around line 14-15: The constructor allows maxSize <= 0 which will create a
zero-length buffer and cause add() to fail; validate maxSize in
DevToolsLogBuffer's constructor (the maxSize parameter) and enforce a positive
value (e.g., require(maxSize > 0)) so arrayOfNulls<DevToolsLog>(maxSize) is
never created with size 0; add a clear error message referencing maxSize in the
precondition to fail-fast during instantiation.
- Around line 37-58: Both add() and clear() create a snapshot under lock but
update the StateFlow _logs.value after releasing the lock, allowing an older
snapshot to overwrite a newer one; move the assignment to _logs.value inside the
lock.withLock block in both add() and clear() so the snapshot creation and
StateFlow update are atomic (i.e., inside lock.withLock in add() after calling
getSnapshotLocked(), and inside lock.withLock in clear() after preparing the
emptyList), referencing methods/fields add(), clear(), lock.withLock,
getSnapshotLocked(), _logs.value, buffer, index, and isFull.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt`:
- Around line 106-108: The export currently writes raw log.message and
log.throwable to disk in ActionsPanel.kt (inside the block that calls
writer.write), which can leak auth/session fragments; add a redaction step by
implementing a helper like redactSensitiveData(text: String): String (or reuse
an existing redactor) that strips/masks known patterns (cookies, auth tokens,
visitorData, dataSyncId, session IDs, long hex/base64 blobs, etc.) and apply it
to log.message and log.throwable?.toString() before formatting/writing; replace
the direct uses of log.message and log.throwable in the writer.write call with
their redacted equivalents so exported files never contain raw sensitive
fragments.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 383-388: The color indicator Box uses Modifier.fillMaxHeight()
inside a Row that lacks explicit height constraints so fillMaxHeight() may be
unconstrained; wrap or modify the parent Row (the Row containing the Box) to use
Modifier.height(IntrinsicSize.Min) (or apply Modifier.height(IntrinsicSize.Min)
to the Row) so the Row measures to its tallest child and the Box's
fillMaxHeight() will expand correctly, ensuring the Box, the Row, and any
siblings render consistent heights; update the Row declaration surrounding the
Box in LogViewerPanel.kt accordingly.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 3121-3127: The cleanup races because discordRpc is read inside a
GlobalScope coroutine while being nulled immediately; instead capture the
reference into a local val (e.g., val rpc = discordRpc), set discordRpc = null
synchronously, and then launch the close on the service's lifecycle scope
(scope) with Dispatchers.IO; perform the suspend close inside runCatching and
log failures (using Timber.tag(TAG).e or similar) to ensure the close runs
reliably and errors are handled rather than using GlobalScope.launch.
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 460-462: The fallbacks for the sleep window in PlayerConnection
are inconsistent with the app settings: replace the hardcoded defaults used when
reading SleepTimerStartTimeKey and SleepTimerEndTimeKey from the DataStore
(currently "09:00" and "23:00") with the settings/dialog defaults "22:00" and
"06:00" so the auto sleep logic matches the UI; keep the existing
SleepTimerDefaultKey handling (30f → roundToInt()) unchanged unless settings
specify a different default minutes value.
In `@app/src/main/kotlin/com/metrolist/music/ui/component/SleepTimerDialog.kt`:
- Around line 420-422: The catch in the initialLocalTime parsing currently
swallows errors and returns a hardcoded LocalTime.of(9, 0); change the fallback
to parse the dialog default by using LocalTime.parse(DEFAULT_START,
timeFormatter) instead so invalid persisted values align with the dialog
default; update the try/catch around LocalTime.parse(initialTime, timeFormatter)
in initialLocalTime (and ensure DEFAULT_START is accessible in this scope) and
keep using the existing DateTimeFormatter timeFormatter.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt`:
- Around line 70-72: DevToolsSettingsScreen lacks a runtime guard so the
"settings/devtools" route can be opened directly; inside the
DevToolsSettingsScreen composable, check the same devMode flag used by
SettingsScreen (e.g., devMode from viewModel or nav arguments) at the start and
if false either call navController.popBackStack() to navigate back or show an
access-denied UI/snackbar and return early to prevent rendering the developer
actions (including the onClick that throws RuntimeException). Ensure you
reference the DevToolsSettingsScreen composable and the devMode boolean (and
navController) so the check mirrors the SettingsScreen gating logic.
- Around line 45-92: The TopAppBar is being declared after the scrollable Column
so it renders below the content; wrap the screen in a Scaffold (or Box) and move
the TopAppBar into the Scaffold's topBar slot so the app bar overlays/sticks to
the top while the Column (the scrollable content that contains SwitchPreference
and PreferenceEntry) is placed in the Scaffold content with the provided
contentPadding (e.g., use Scaffold(topBar = { TopAppBar(...) }, content = {
padding ->
Column(Modifier.padding(padding).windowInsetsPadding(...).verticalScroll(...)) {
... } })). Ensure you keep the same TopAppBar parameters (title, navigationIcon
using navController::navigateUp and navController::backToMain, scrollBehavior)
and preserve the existing Column modifiers and children.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/PlayerSettings.kt`:
- Around line 519-522: The default value for the sleep timer preference is
"6:00", which fails LocalTime.parse(...) in PlayerConnection (uses
DateTimeFormatter.ofPattern("HH:mm")); update the rememberPreference call for
SleepTimerEndTimeKey (the sleepTimerEndTime default in PlayerSettings, where
rememberPreference is invoked) to use a zero-padded hour string "06:00" so
parsing with "HH:mm" succeeds at startup.
In `@app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt`:
- Line 43: ScrobbleManager currently logs raw song titles via
Timber.tag(...).d("onSongStart: ${metadata.title}") (and similar calls in
onSongEnd/other handlers using metadata.title); change this so these debug/info
logs are only emitted in dev builds by either gating each Timber.d call with if
(BuildConfig.DEBUG) or by ensuring the Timber tree that records/export logs
(e.g., the DevTools log tree) is only planted when BuildConfig.DEBUG == true;
also verify the DevTools in-app log capture/export endpoint is disabled or
unreachable in release builds so release builds never record/export debug-level
messages.
In `@app/src/main/kotlin/com/metrolist/music/utils/Utils.kt`:
- Line 14: reportException calls Timber.e(throwable, "Caught exception in
reportException") but Timber is only planted in debug builds (App.kt uses
BuildConfig.DEBUG), so exceptions are dropped in release; fix by ensuring a
logging tree is always planted (e.g., implement and plant a ReleaseTree in
App.kt when !BuildConfig.DEBUG) or add a fallback inside reportException to
forward the throwable to another logger (e.g., Log.e or your crash-reporting
SDK) when Timber.treeCount == 0; update App.kt planting logic and/or modify
reportException to check Timber.forest() or Timber.treeCount and handle the
no-tree case to guarantee exceptions are recorded.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 30-33: In updateSong, validate playbackSpeed before any timestamp
math: ensure playbackSpeed > 0 (or default to 1.0f) and handle non-positive
values early to avoid dividing by/using zero or negative speed when computing
durationMillis, startTimestamp and endTimestamp; update the logic around
currentPlaybackTimeMillis, durationMillis and the presence start/end timestamp
calculations to use the validated/clamped playbackSpeed (or bail out/skip
presence update) so timestamps remain valid; reference function updateSong,
parameters currentPlaybackTimeMillis and playbackSpeed, and the presence
timestamp computations (startTimestamp/endTimestamp) when making the change.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt`:
- Around line 22-23: The
`@OptIn`(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class)
annotation on the DatabaseInfoPanel composable is unnecessary; remove that
annotation from DatabaseInfoPanel unless the called composable PanelHeader
actually requires the opt-in—if PanelHeader requires it, move the `@OptIn` to the
call site of PanelHeader (or annotate PanelHeader itself) instead of annotating
DatabaseInfoPanel, so only the functions that use
ExperimentalMaterial3ExpressiveApi are opting in.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`:
- Around line 52-58: DevToolsFabBottomPadding manually fetches a dimension and
converts it to Dp; replace that logic with Compose's dimensionResource to
simplify and remove manual density conversion: inside DevToolsFabBottomPadding
use
androidx.compose.ui.res.dimensionResource(R.dimen.devtools_fab_bottom_padding)
(or import dimensionResource) and return it as Dp, removing
LocalContext/LocalDensity and the with(density){...} conversion.
- Around line 115-134: The FAB's position (offsetX, offsetY) is only remembered
in-memory and resets on configuration changes; update DevToolsOverlay to persist
these values by replacing remember with rememberSaveable for offsetX and offsetY
(and add the import androidx.compose.runtime.saveable.rememberSaveable),
ensuring the drag logic in the FloatingActionButton still uses the same
variables (offsetX, offsetY) and bounds checking so position survives rotations
and process restarts.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 376-380: The Surface click currently toggles selection when
isSelected and otherwise toggles expanded, which prevents expanding a selected
log; update the interaction so expand/collapse is independent of selection and
move selection to a long-press: replace the single Modifier.clickable usage in
LogViewerPanel's Surface with a combinedClickable (or similar) that always
toggles expanded (flip the expanded state) on normal click and calls
onToggleSelect(log.id) onLongClick; also remove or hide the header IconButton
selection toggle (the header IconButton referenced in the row header block) if
you adopt long-press selection to avoid duplicate controls and visual clutter.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt`:
- Around line 19-20: Remove the unnecessary
`@OptIn`(androidx.compose.material3.ExperimentalMaterial3ExpressiveApi::class)
annotations applied to the composable declarations for InfoCard and InfoRow;
locate the annotations immediately above the `@Composable` fun InfoCard(...) and
`@Composable` fun InfoRow(...) and delete those `@OptIn` lines so the composables
compile without opting into ExperimentalMaterial3ExpressiveApi.
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 431-442: The parseDayTimes function duplicates logic in
SleepTimerDialog; extract parseDayTimes into a shared non-UI utility (e.g., a
new object or util file) and have both PlayerConnection.parseDayTimes and
SleepTimerDialog use that single function; move the logic currently in private
fun parseDayTimes(raw: String): Map<Int, Pair<String, String>> into the new
utility (preserving signature or providing a compatible public function),
replace the private function in PlayerConnection with a call to the shared util,
and update SleepTimerDialog to call the same util so serialization/parsing logic
is centralized and reused.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 27-28: The debug logs in DiscordRPC are emitting song metadata;
update the Timber.d calls in the DiscordRPC class (remove or redact song
title/artist fields) so they no longer log plaintext song metadata—e.g., in the
methods that currently log track info (the Timber.d calls that reference song
title/artist, such as the presence/update handlers or track-change methods),
replace the logged title/artist with a redacted placeholder or log only
non-sensitive identifiers (e.g., a track ID or boolean flags) while keeping the
existing masked token log intact.
In `@app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt`:
- Line 36: Multiple Timber.tag("LastFM") usages in ScrobbleManager are repeated
string literals; add a single constant (e.g., private const val TAG = "LastFM")
in ScrobbleManager (companion object or top-level) and update all calls like
Timber.tag("LastFM").d(...) / Timber.tag("LastFM").i(...) etc. to use
Timber.tag(TAG) so the tag is centralized and avoids typos (affects occurrences
referenced in ScrobbleManager).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
app/src/main/kotlin/com/metrolist/music/App.ktapp/src/main/kotlin/com/metrolist/music/MainActivity.ktapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/db/DatabaseDao.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/component/SleepTimerDialog.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/PlayerSettings.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/drawable/baseline_event_repeat_24.xmlapp/src/main/res/values/metrolist_strings.xmlapp/src/main/res/values/values.xml
| if (metadata == null) return | ||
| songStartedAt = System.currentTimeMillis() / 1000 | ||
| songStarted = true | ||
| Timber.tag("LastFM").d("onSongStart: ${metadata.title}") |
There was a problem hiding this comment.
Gate track-title logs to dev-only paths to prevent exposure of listening history.
At lines 43, 51, 122, and 135, song titles from metadata.title are logged unconditionally with Timber. With in-app log capture/export functionality via the DevTools overlay, these debug logs could expose user listening history in non-debug builds unless Timber trees are strictly gated to debug/developer mode only.
Verify that:
- Timber trees capturing DEBUG logs (especially any
DevTools-related tree) are only planted whenBuildConfig.DEBUG == trueor equivalent dev gating - Release/production builds do not plant trees that record or export debug-level messages
- The in-app log capture/export endpoint is not accessible in release builds
Also applies to: lines 51-51, 122-122, 135-135
🤖 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/ScrobbleManager.kt` at line 43,
ScrobbleManager currently logs raw song titles via
Timber.tag(...).d("onSongStart: ${metadata.title}") (and similar calls in
onSongEnd/other handlers using metadata.title); change this so these debug/info
logs are only emitted in dev builds by either gating each Timber.d call with if
(BuildConfig.DEBUG) or by ensuring the Timber tree that records/export logs
(e.g., the DevTools log tree) is only planted when BuildConfig.DEBUG == true;
also verify the DevTools in-app log capture/export endpoint is disabled or
unreachable in release builds so release builds never record/export debug-level
messages.
|
|
||
| fun reportException(throwable: Throwable) { | ||
| throwable.printStackTrace() | ||
| Timber.e(throwable, "Caught exception in reportException") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where Timber trees are planted
echo "=== SEARCHING FOR TIMBER.PLANT CALLS ==="
rg -nP --type=kotlin -C2 '\bTimber\.plant\s*\(' 2>/dev/null || echo "No Timber.plant calls found"
echo ""
echo "=== SEARCHING FOR REPORTEXCEPTION CALL SITES ==="
# 2) Find all call sites of reportException
rg -nP --type=kotlin -C2 '\breportException\s*\(' 2>/dev/null || echo "No reportException calls found"
echo ""
echo "=== INSPECTING APP ENTRYPOINTS ==="
# 3) Inspect common app entrypoints for startup planting
fd -e kt 'App|Application' app/src/main/kotlin 2>/dev/null | head -20 | while read file; do
echo "=== $file ==="
rg -nP -C2 'onCreate\s*\(|Timber\.plant\s*\(' "$file" 2>/dev/null || true
doneRepository: adrielGGmotion/Metrolist
Length of output: 42635
Timber is not planted in release builds, causing silent exception loss.
The Timber trees are only planted within the BuildConfig.DEBUG condition in App.kt (lines 75-76). In release/production builds, no tree is planted, so when reportException() calls Timber.e(), the exception is silently discarded instead of being logged. Consider ensuring a tree is always planted (e.g., a ReleaseTree for production) or add a fallback in reportException() to handle the no-tree case.
🤖 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/Utils.kt` at line 14,
reportException calls Timber.e(throwable, "Caught exception in reportException")
but Timber is only planted in debug builds (App.kt uses BuildConfig.DEBUG), so
exceptions are dropped in release; fix by ensuring a logging tree is always
planted (e.g., implement and plant a ReleaseTree in App.kt when
!BuildConfig.DEBUG) or add a fallback inside reportException to forward the
throwable to another logger (e.g., Log.e or your crash-reporting SDK) when
Timber.treeCount == 0; update App.kt planting logic and/or modify
reportException to check Timber.forest() or Timber.treeCount and handle the
no-tree case to guarantee exceptions are recorded.
![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 significant new feature: a developer tools overlay for debugging, which includes a log viewer, player state inspector, database information, and various utility actions. This is a well-implemented and valuable addition for development and troubleshooting. The PR also includes a new automatic sleep timer feature with extensive configuration options. The code is generally of high quality, but I have a few suggestions to improve maintainability and adhere to best practices, particularly regarding coroutine scope management and reducing code complexity in a few areas.
Comment on lines
+115
to
+123
| val tagMatch = selectedTagGroups.isEmpty() || selectedTagGroups.any { group -> | ||
| when (group) { | ||
| tagPlayer -> log.tag.contains("Player") || log.tag.contains("ExoPlayer") || log.tag.contains("MusicService") | ||
| tagUi -> log.tag.contains("Screen") || log.tag.contains("Activity") | ||
| tagDb -> log.tag.contains("Room") || log.tag.contains("Database") || log.tag.contains("Dao") | ||
| tagIntegration -> log.tag.contains("Discord") || log.tag.contains("LastFM") || log.tag.contains("Kizzy") | ||
| else -> true | ||
| } | ||
| } |
There was a problem hiding this comment.
The filtering logic for tag groups uses hardcoded strings like "Player", "ExoPlayer", "MusicService", etc. This can make the code harder to maintain. It would be better to define these tag keywords as constants in a companion object or a separate constants file. This improves readability and makes it easier to update the tags in one place.
For example:
private object LogTagGroups {
val PLAYER = listOf("Player", "ExoPlayer", "MusicService")
val UI = listOf("Screen", "Activity")
val DB = listOf("Room", "Database", "Dao")
val INTEGRATION = listOf("Discord", "LastFM", "Kizzy")
}
...
when (group) {
tagPlayer -> LogTagGroups.PLAYER.any { log.tag.contains(it, ignoreCase = true) }
tagUi -> LogTagGroups.UI.any { log.tag.contains(it, ignoreCase = true) }
...
}
Comment on lines
+3122
to
+3125
| @OptIn(kotlinx.coroutines.DelicateCoroutinesApi::class) | ||
| kotlinx.coroutines.GlobalScope.launch { | ||
| discordRpc?.close() | ||
| } |
There was a problem hiding this comment.
Using GlobalScope is generally discouraged as it can lead to memory leaks and hard-to-track work. While it's used here in onDestroy where the service's own scope is being cancelled, a better practice is to inject an application-level CoroutineScope that lives as long as the application itself. This provides better structure and testability for background work that needs to outlive a specific component.
Your AppModule already provides an @ApplicationScope. You can inject this into the MusicService and use it to launch this coroutine.
Suggested change
| @OptIn(kotlinx.coroutines.DelicateCoroutinesApi::class) | |
| kotlinx.coroutines.GlobalScope.launch { | |
| discordRpc?.close() | |
| } | |
| @OptIn(kotlinx.coroutines.DelicateCoroutinesApi::class) | |
| scope.launch { | |
| discordRpc?.close() | |
| } |
Comment on lines
+444
to
+530
| private fun checkAndStartAutomaticSleepTimer(): Boolean { | ||
| return try { | ||
| val sleepTimerEnabled = service.applicationContext.dataStore.get(SleepTimerEnabledKey) ?: false | ||
| Timber.tag(TAG).d("✓ Sleep Timer Check: enabled=$sleepTimerEnabled") | ||
|
|
||
| if (!sleepTimerEnabled) { | ||
| Timber.tag(TAG).d("✗ Sleep Timer disabled - skipping") | ||
| return false | ||
| } | ||
|
|
||
| if (service.sleepTimer.isActive) { | ||
| Timber.tag(TAG).d("✗ Sleep Timer already active - skipping") | ||
| return false | ||
| } | ||
|
|
||
| val sleepTimerRepeat = service.applicationContext.dataStore.get(SleepTimerRepeatKey) ?: "daily" | ||
| val sleepTimerStartTime = service.applicationContext.dataStore.get(SleepTimerStartTimeKey) ?: "09:00" | ||
| val sleepTimerEndTime = service.applicationContext.dataStore.get(SleepTimerEndTimeKey) ?: "23:00" | ||
| val sleepTimerDefaultMinutes = (service.applicationContext.dataStore.get(SleepTimerDefaultKey) ?: 30f).roundToInt() | ||
| val sleepTimerCustomDaysStr = service.applicationContext.dataStore.get(SleepTimerCustomDaysKey) ?: "0,1,2,3,4" | ||
| val sleepTimerDayTimesStr = service.applicationContext.dataStore.get(SleepTimerDayTimesKey) ?: "" | ||
|
|
||
| Timber.tag(TAG).d("Sleep Timer Config: repeat=$sleepTimerRepeat start=$sleepTimerStartTime end=$sleepTimerEndTime default=$sleepTimerDefaultMinutes custom=$sleepTimerCustomDaysStr") | ||
|
|
||
| val currentTime = LocalTime.now() | ||
| val today = LocalDate.now() | ||
| val dayOfWeek = today.dayOfWeek.value % 7 | ||
| val adjustedDayOfWeek = if (dayOfWeek == 0) 6 else dayOfWeek - 1 | ||
|
|
||
| Timber.tag(TAG).d("Current: time=$currentTime dayOfWeek=$adjustedDayOfWeek") | ||
|
|
||
| val isDayAllowed = when (sleepTimerRepeat) { | ||
| "daily" -> true | ||
| "weekdays" -> adjustedDayOfWeek in 0..4 | ||
| "weekends" -> adjustedDayOfWeek in 5..6 | ||
| "weekdays_weekends" -> true // both groups active; per-day time handles the distinction | ||
| "custom" -> { | ||
| val customDays = sleepTimerCustomDaysStr.split(",").mapNotNull { it.trim().toIntOrNull() } | ||
| Timber.tag(TAG).d("Custom days: $customDays, adjustedDayOfWeek=$adjustedDayOfWeek") | ||
| adjustedDayOfWeek in customDays | ||
| } | ||
| else -> false | ||
| } | ||
|
|
||
| if (!isDayAllowed) { | ||
| Timber.tag(TAG).d("✗ Day not allowed for Sleep Timer") | ||
| return false | ||
| } | ||
|
|
||
| // "daily" uses the single global time window. | ||
| // All other modes store per-day times in the dayTimes map so that | ||
| // e.g. weekdays and weekends can have different windows. | ||
| val timeFormatter = DateTimeFormatter.ofPattern("HH:mm") | ||
| val usesDayTimesMap = sleepTimerRepeat != "daily" | ||
| val (startStr, endStr) = if (usesDayTimesMap) { | ||
| parseDayTimes(sleepTimerDayTimesStr)[adjustedDayOfWeek] | ||
| ?: (sleepTimerStartTime to sleepTimerEndTime) | ||
| } else { | ||
| sleepTimerStartTime to sleepTimerEndTime | ||
| } | ||
|
|
||
| val startTime = LocalTime.parse(startStr, timeFormatter) | ||
| val endTime = LocalTime.parse(endStr, timeFormatter) | ||
|
|
||
| // Support overnight ranges (e.g. 22:00–06:00) in addition to normal ranges | ||
| val isTimeInRange = if (endTime.isAfter(startTime)) { | ||
| currentTime.isAfter(startTime) && currentTime.isBefore(endTime) | ||
| } else { | ||
| currentTime.isAfter(startTime) || currentTime.isBefore(endTime) | ||
| } | ||
|
|
||
| Timber.tag(TAG).d("Time check: $currentTime between $startStr-$endStr? $isTimeInRange") | ||
|
|
||
| if (isTimeInRange) { | ||
| Timber.tag(TAG).i("AUTO SLEEP TIMER STARTED: $sleepTimerDefaultMinutes minutes") | ||
| service.sleepTimer.start(sleepTimerDefaultMinutes) | ||
| return true | ||
| } | ||
|
|
||
| Timber.tag(TAG).d("✗ Time not in range") | ||
| return false | ||
|
|
||
| } catch (e: Exception) { | ||
| Timber.tag(TAG).e(e, "Sleep Timer error") | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
This function is quite long and handles multiple responsibilities (reading settings, checking day/time, parsing strings, starting the timer). To improve readability and maintainability, consider refactoring it into smaller, more focused private functions. For example:
- A function to read and hold the sleep timer configuration.
- A function to check if the current day is allowed based on the configuration.
- A function to determine the correct start/end time for the current day.
- A function to check if the current time is within the given range.
This will make the logic easier to follow and test.
Comment on lines
+352
to
+372
| val (finalRepeat, finalDayTimes) = when (selectedRepeat) { | ||
| "weekdays_weekends" -> { | ||
| // Collapse the two booleans back into a single string value | ||
| val repeat = when { | ||
| weekdaysEnabled && weekendsEnabled -> "weekdays_weekends" | ||
| weekdaysEnabled -> "weekdays" | ||
| weekendsEnabled -> "weekends" | ||
| else -> "daily" // nothing checked, fall back | ||
| } | ||
| val times = buildMap { | ||
| if (weekdaysEnabled) { | ||
| for (d in WEEKDAY_INDICES) put(d, weekdaysStart to weekdaysEnd) | ||
| } | ||
| if (weekendsEnabled) { | ||
| for (d in WEEKEND_INDICES) put(d, weekendsStart to weekendsEnd) | ||
| } | ||
| } | ||
| repeat to times | ||
| } | ||
| else -> selectedRepeat to dayTimesMap.toMap() | ||
| } |
There was a problem hiding this comment.
The logic in the onConfirm lambda to determine the final repeat value and dayTimes map is quite complex, as it reconstructs state from several Boolean flags (weekdaysEnabled, weekendsEnabled). This could be simplified by using a state holder class or a data class to represent the dialog's configuration. The UI would update this state object directly, and the onConfirm lambda would simply pass the final state back. This would make the logic more declarative and easier to reason about.
Example state holder:
data class SleepTimerConfig(
val repeatMode: String,
val weekdaysEnabled: Boolean,
val weekendsEnabled: Boolean,
// ... other fields
)
// In composable:
var config by remember { mutableStateOf(initialConfig) }
...
// onConfirm would just pass 'config'
adrielGGmotion
force-pushed
the
feature/devtools
branch
from
March 2, 2026 19:03
2b44ed0 to
2cde56b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/utils/Utils.kt (1)
14-14:⚠️ Potential issue | 🟠 MajorEnsure
reportExceptionstill records errors when no Timber tree is planted.At Line 14,
Timber.e(...)is fine only if a tree is always planted; otherwise release exceptions can be silently dropped. Please either guarantee a release tree in app startup or add a fallback inreportExceptionwhenTimber.treeCount == 0.#!/bin/bash set -euo pipefail echo "=== Timber planting points ===" rg -nP --type=kotlin -C2 '\bTimber\.plant\s*\(' echo echo "=== App/Application startup files ===" fd -e kt 'App|Application' app/src/main/kotlin | while read -r f; do echo "--- $f ---" rg -nP -C2 'onCreate\s*\(|Timber\.plant\s*\(' "$f" || true done echo echo "=== reportException implementation ===" rg -nP --type=kotlin -C3 '\bfun\s+reportException\s*\('Expected: at least one non-debug planting path (or explicit fallback behavior in
reportException).🤖 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/Utils.kt` at line 14, reportException currently calls Timber.e(...) which drops logs when no Timber tree is planted; update reportException to check Timber.treeCount and if it's 0 use a fallback (e.g., call android.util.Log.e with the throwable and message and/or forward the throwable to your crash/telemetry client) so errors are still recorded in release builds, or alternatively ensure a non-debug Timber tree is always planted in your App.onCreate startup path; reference reportException, Timber.treeCount, and Timber.e when making the change.
🧹 Nitpick comments (6)
app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt (1)
118-119: Mark helper composables asprivateto reduce API surface.
QueueViewerRow(line 119) andTimelineRow(line 162) are only called within this file and should be markedprivateto narrow their visibility.♻️ Proposed change
`@Composable` -fun QueueViewerRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { +private fun QueueViewerRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { `@Composable` -fun TimelineRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { +private fun TimelineRow(playerConnection: com.metrolist.music.playback.PlayerConnection) {🤖 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 118 - 119, Mark the internal helper composables as private to reduce the file's public API: change the declarations of QueueViewerRow (currently fun QueueViewerRow(playerConnection: com.metrolist.music.playback.PlayerConnection)) and TimelineRow to be private functions (private fun ...) so they are only visible within this file; update both function headers accordingly and ensure any callers in this file remain unchanged.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
354-379: Consider providing earlier feedback during the tap sequence.The Easter egg requires 9 taps but only shows toast feedback for the last 3 taps (when
remainingis 1–3). Users might think their taps aren't registering during the first 6 taps since there's no visual feedback.Consider either:
- Showing a brief indication after the first few taps (e.g., "Keep tapping...")
- Adjusting
DEV_MODE_COUNTDOWN_STARTto a higher valueThis is a minor UX concern and optional to address.
🤖 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 354 - 379, The tap-to-enable developer mode only shows Toast feedback when remaining taps are within DEV_MODE_COUNTDOWN_START (currently last few taps), which can confuse users during earlier taps; update the logic in the clickable handler (symbols: isDeveloperModeEnabled, DEV_MODE_TAP_TIMEOUT_MS, tapCount, DEV_MODE_REQUIRED_TAPS, DEV_MODE_COUNTDOWN_START, remaining) to provide earlier feedback by either increasing DEV_MODE_COUNTDOWN_START or adding an extra branch that shows a subtle Toast (e.g., "Keep tapping...") when tapCount crosses a lower threshold (or every N taps) before the final countdown, and keep the existing coroutineScope.edit call that sets DeveloperModeKey when remaining <= 0.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (1)
95-101: Consider navigating back when developer mode is disabled.When the user disables developer mode via this switch, they remain on the DevTools screen (though the guard shows "dev mode required"). For better UX, consider navigating back automatically:
♻️ Suggested improvement
+ val coroutineScope = rememberCoroutineScope() + + LaunchedEffect(devMode) { + if (!devMode) { + navController.navigateUp() + } + } + if (!devMode) { Box( modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center ) { Text(stringResource(R.string.dev_mode_required)) } return }🤖 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 95 - 101, When the user toggles developer mode off in SwitchPreference (checked = devMode, onCheckedChange = { devMode = it }), update the handler to both update devMode and navigate back so the DevTools screen is not left visible when access is revoked; modify the onCheckedChange to set devMode = it and if it == false call the appropriate navigation action (e.g., navController.popBackStack() or navController.navigateUp(), or invoke an onNavigateBack/onClose callback provided to DevToolsSettingsScreen) so the screen is dismissed immediately after disabling developer mode.app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt (2)
468-472: Day-of-week mapping is correct but could use a clarifying comment.The calculation maps
java.time.DayOfWeek(Monday=1 to Sunday=7) to a 0-indexed Monday-based week (Monday=0 to Sunday=6). The logic works but is non-obvious at first glance.📝 Suggested clarification
val currentTime = LocalTime.now() val today = LocalDate.now() + // java.time.DayOfWeek: Monday=1, Sunday=7 + // Convert to 0-indexed Monday-based: Monday=0, Sunday=6 val dayOfWeek = today.dayOfWeek.value % 7 val adjustedDayOfWeek = if (dayOfWeek == 0) 6 else dayOfWeek - 1🤖 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/PlayerConnection.kt` around lines 468 - 472, Add a brief clarifying comment above the block that computes currentTime/today/dayOfWeek/adjustedDayOfWeek in PlayerConnection.kt explaining that java.time.DayOfWeek.value returns 1..7 (Monday..Sunday) and the logic maps that to a 0-indexed, Monday-based range (0..6) by taking value % 7 and converting the Sunday result (0) to 6; keep the existing computation unchanged and reference the variables currentTime, today, dayOfWeek, and adjustedDayOfWeek in the comment for clarity.
444-530: Potential blocking I/O on playback thread.The
checkAndStartAutomaticSleepTimer()function performs multiple synchronousdataStore.get()calls (lines 446-464). If this runs on the main thread or a time-sensitive playback thread duringonPlayWhenReadyChanged, it could cause UI jank or playback hiccups.Consider wrapping this in a coroutine or moving the preference reads to a background context:
♻️ Suggested approach
override fun onPlayWhenReadyChanged( newPlayWhenReady: Boolean, reason: Int, ) { val wasPlaying = playWhenReady.value playWhenReady.value = newPlayWhenReady // Central sleep timer trigger: fires on every paused -> playing transition, if (newPlayWhenReady && !wasPlaying) { - checkAndStartAutomaticSleepTimer() + scope.launch(Dispatchers.IO) { + checkAndStartAutomaticSleepTimer() + } } }Where
scopeis theCoroutineScopeavailable inPlayerConnection.🤖 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/PlayerConnection.kt` around lines 444 - 530, checkAndStartAutomaticSleepTimer currently performs synchronous service.applicationContext.dataStore.get(...) calls on the caller thread (likely the playback/main thread); move all DataStore reads off the playback thread by making the work run in a coroutine on a background dispatcher (e.g., scope.launch or withContext(Dispatchers.IO) from the PlayerConnection's scope), gather the preferences there, then invoke service.sleepTimer.start(...) back on the appropriate thread if needed; you can either make checkAndStartAutomaticSleepTimer a suspend function or create a small non-blocking wrapper that launches the background coroutine and returns asynchronously, and update callers (e.g., onPlayWhenReadyChanged) to use the coroutine-based variant.app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
103-107: Prefer stable tag-group keys over localized strings for state.
selectedTagGroupscurrently stores translated labels. Using an enum/sealed key for state and mapping to labels only at render time is safer and easier to maintain.Also applies to: 116-123, 275-286
🤖 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 103 - 107, selectedTagGroups is storing localized strings (tagPlayer, tagUi, tagDb, tagIntegration via stringResource) which is fragile; change state to store a stable key type (e.g., a TagGroup enum or sealed class) instead of translated labels, update the code paths that read/modify selectedTagGroups (including usages around the current block and the ranges referenced at 116-123 and 275-286) to work with the enum values, and only call stringResource to map the enum to a localized label at render time (e.g., when building UI labels or menu entries) so state comparisons and persistence use the stable enum keys rather than localized text.
🤖 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/DevToolsOverlay.kt`:
- Around line 122-123: The Y-axis drag bounds can become inverted causing
coerceIn to throw; compute the bounds first (e.g., minY = -screenHeightPx/2f +
dragBoundsPaddingPx and maxY = screenHeightPx/2f - dragBoundsPaddingPx) and
guard against inversion before calling coerceIn on offsetY (in the same block
that updates offsetY using dragAmount.y). If minY > maxY, normalize them to a
single value (for example set minY = maxY = (minY + maxY) / 2f) or set minY =
maxY = 0f, then call offsetY = (offsetY + dragAmount.y).coerceIn(minY, maxY) so
coerceIn always receives a valid range.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 86-88: When logs are removed/cleared the selectedLogIds set is not
pruned, producing stale selections; update the places that mutate or clear the
logs buffer (the code that clears the logs list / buffer and any code that
updates the logs collection) to also update selectedLogIds by either clearing it
or intersecting it with the current set of existing log IDs (use selectedLogIds
/ selectedLogIdsState), e.g. replace kept selections after a clear with
selectedLogIds = emptySet() or selectedLogIds =
selectedLogIds.intersect(currentLogs.map { it.id }.toSet()) so the selection
count always reflects live rows.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt`:
- Around line 104-106: Replace raw enum.name usages in the InfoRow calls by
mapping ConnectionState and RoomRole to localized string resources: compute a
statusText from the connectionState (e.g., mapping ConnectionState.CONNECTED,
CONNECTING, RECONNECTING, ERROR, DISCONNECTED to R.string.listen_together_* via
stringResource) and compute a roleText from role (RoomRole.HOST, GUEST, NONE to
R.string.listen_together_*), then pass those localized strings into InfoRow
instead of connectionState.name and role.name; update the InfoRow invocations in
PlayerStatePanel (where connectionState and role are used) to use the mapped
statusText and roleText.
- Around line 171-172: Normalize Media3 sentinel values before formatting: for
any use of playerConnection.player.duration,
playerConnection.player.currentPosition, and
playerConnection.player.bufferedPosition (and any call sites that pass those
values into your time-formatting logic/formatTime function in
PlayerStatePanel.kt), check for androidx.media3.common.C.TIME_UNSET and replace
it with a safe value (e.g., 0L or null-handled) before rendering the timeline
row so negative sentinel values are not displayed; update the assignments around
duration = playerConnection.player.duration and the similar uses at the other
locations (currentPosition/bufferedPosition) to perform this check and pass only
normalized values into the UI formatting code.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 405-408: The unguarded calls to discordRpc?.close() (e.g., the
call inside the scope.launch(Dispatchers.IO) block where
Timber.tag(TAG).d("Discord RPC: screen off while paused, closing connection") is
logged, and the other sites flagged) can throw and cancel the service scope;
wrap each call in runCatching { discordRpc?.close() }.onFailure {
Timber.tag(TAG).e(it, "Failed to close Discord RPC") } (or similar logging) so
exceptions are caught and logged; apply this pattern at every site where close()
is invoked (including the calls referenced in the comment) and preserve the
surrounding coroutine/context (e.g., inside scope.launch blocks).
In `@app/src/main/res/values/metrolist_strings.xml`:
- Around line 286-289: The four string resources have awkward English; update
the values of string names sleep_timer_description,
sleep_timer_repeat_description, sleep_timer_activate, and sleep_timer_repeat to
clearer phrasing: change sleep_timer_description to "Enable the sleep timer and
set a custom duration", sleep_timer_repeat_description to "Choose days and a
time for the sleep timer to activate automatically", sleep_timer_activate to
"The sleep timer will activate automatically for songs played between the start
and end times", and keep sleep_timer_repeat as "Repeat" (or change to "Repeat
days" if clearer for UI); edit these string values in metrolist_strings.xml
accordingly.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/utils/Utils.kt`:
- Line 14: reportException currently calls Timber.e(...) which drops logs when
no Timber tree is planted; update reportException to check Timber.treeCount and
if it's 0 use a fallback (e.g., call android.util.Log.e with the throwable and
message and/or forward the throwable to your crash/telemetry client) so errors
are still recorded in release builds, or alternatively ensure a non-debug Timber
tree is always planted in your App.onCreate startup path; reference
reportException, Timber.treeCount, and Timber.e when making the change.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 103-107: selectedTagGroups is storing localized strings
(tagPlayer, tagUi, tagDb, tagIntegration via stringResource) which is fragile;
change state to store a stable key type (e.g., a TagGroup enum or sealed class)
instead of translated labels, update the code paths that read/modify
selectedTagGroups (including usages around the current block and the ranges
referenced at 116-123 and 275-286) to work with the enum values, and only call
stringResource to map the enum to a localized label at render time (e.g., when
building UI labels or menu entries) so state comparisons and persistence use the
stable enum keys rather than localized text.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt`:
- Around line 118-119: Mark the internal helper composables as private to reduce
the file's public API: change the declarations of QueueViewerRow (currently fun
QueueViewerRow(playerConnection: com.metrolist.music.playback.PlayerConnection))
and TimelineRow to be private functions (private fun ...) so they are only
visible within this file; update both function headers accordingly and ensure
any callers in this file remain unchanged.
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 468-472: Add a brief clarifying comment above the block that
computes currentTime/today/dayOfWeek/adjustedDayOfWeek in PlayerConnection.kt
explaining that java.time.DayOfWeek.value returns 1..7 (Monday..Sunday) and the
logic maps that to a 0-indexed, Monday-based range (0..6) by taking value % 7
and converting the Sunday result (0) to 6; keep the existing computation
unchanged and reference the variables currentTime, today, dayOfWeek, and
adjustedDayOfWeek in the comment for clarity.
- Around line 444-530: checkAndStartAutomaticSleepTimer currently performs
synchronous service.applicationContext.dataStore.get(...) calls on the caller
thread (likely the playback/main thread); move all DataStore reads off the
playback thread by making the work run in a coroutine on a background dispatcher
(e.g., scope.launch or withContext(Dispatchers.IO) from the PlayerConnection's
scope), gather the preferences there, then invoke service.sleepTimer.start(...)
back on the appropriate thread if needed; you can either make
checkAndStartAutomaticSleepTimer a suspend function or create a small
non-blocking wrapper that launches the background coroutine and returns
asynchronously, and update callers (e.g., onPlayWhenReadyChanged) to use the
coroutine-based variant.
In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`:
- Around line 354-379: The tap-to-enable developer mode only shows Toast
feedback when remaining taps are within DEV_MODE_COUNTDOWN_START (currently last
few taps), which can confuse users during earlier taps; update the logic in the
clickable handler (symbols: isDeveloperModeEnabled, DEV_MODE_TAP_TIMEOUT_MS,
tapCount, DEV_MODE_REQUIRED_TAPS, DEV_MODE_COUNTDOWN_START, remaining) to
provide earlier feedback by either increasing DEV_MODE_COUNTDOWN_START or adding
an extra branch that shows a subtle Toast (e.g., "Keep tapping...") when
tapCount crosses a lower threshold (or every N taps) before the final countdown,
and keep the existing coroutineScope.edit call that sets DeveloperModeKey when
remaining <= 0.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt`:
- Around line 95-101: When the user toggles developer mode off in
SwitchPreference (checked = devMode, onCheckedChange = { devMode = it }), update
the handler to both update devMode and navigate back so the DevTools screen is
not left visible when access is revoked; modify the onCheckedChange to set
devMode = it and if it == false call the appropriate navigation action (e.g.,
navController.popBackStack() or navController.navigateUp(), or invoke an
onNavigateBack/onClose callback provided to DevToolsSettingsScreen) so the
screen is dismissed immediately after disabling developer mode.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
app/src/main/kotlin/com/metrolist/music/App.ktapp/src/main/kotlin/com/metrolist/music/MainActivity.ktapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/db/DatabaseDao.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/component/SleepTimerDialog.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/PlayerSettings.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/drawable/baseline_event_repeat_24.xmlapp/src/main/res/values/metrolist_strings.xmlapp/src/main/res/values/values.xml
🚧 Files skipped from review as they are similar to previous changes (8)
- app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt
- app/src/main/kotlin/com/metrolist/music/App.kt
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLog.kt
- app/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.kt
- app/src/main/res/drawable/baseline_event_repeat_24.xml
- app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt
- app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLogBuffer.kt
Comment on lines
+104
to
+106
| InfoRow(stringResource(R.string.dev_listen_together_status), connectionState.name) | ||
| InfoRow(stringResource(R.string.dev_listen_together_room), roomCode) | ||
| InfoRow(stringResource(R.string.dev_listen_together_role), role.name) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "PlayerStatePanel.kt" -t fRepository: adrielGGmotion/Metrolist
Length of output: 139
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt | head -120Repository: adrielGGmotion/Metrolist
Length of output: 6619
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt | sed -n '90,120p'Repository: adrielGGmotion/Metrolist
Length of output: 1979
🏁 Script executed:
rg "listenTogetherManager" -t kt --max-count 20Repository: adrielGGmotion/Metrolist
Length of output: 94
🏁 Script executed:
rg "enum class.*ConnectionState|data class.*ConnectionState" -t ktRepository: adrielGGmotion/Metrolist
Length of output: 94
🏁 Script executed:
rg "class.*ListenTogetherManager|interface.*ListenTogetherManager" -t ktRepository: adrielGGmotion/Metrolist
Length of output: 94
🏁 Script executed:
rg "listenTogetherManager" --max-count 20Repository: adrielGGmotion/Metrolist
Length of output: 15226
🏁 Script executed:
rg "enum class.*ConnectionState|enum class.*Role" --max-count 20Repository: adrielGGmotion/Metrolist
Length of output: 276
🏁 Script executed:
fd "ListenTogether" -t fRepository: adrielGGmotion/Metrolist
Length of output: 645
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/listentogether/ListenTogetherClient.kt | head -100Repository: adrielGGmotion/Metrolist
Length of output: 3613
🏁 Script executed:
rg "dev_listen_together" -A 2Repository: adrielGGmotion/Metrolist
Length of output: 2054
🏁 Script executed:
rg "RoomRole\.|ConnectionState\." --max-count 30Repository: adrielGGmotion/Metrolist
Length of output: 14617
Map enum values to localized strings instead of using .name.
connectionState.name and role.name output raw enum constants. The codebase already defines localized strings for ConnectionState (e.g., R.string.listen_together_connected, R.string.listen_together_connecting). Use the same pattern here:
val statusText = when (connectionState) {
ConnectionState.CONNECTED -> stringResource(R.string.listen_together_connected)
ConnectionState.CONNECTING -> stringResource(R.string.listen_together_connecting)
ConnectionState.RECONNECTING -> stringResource(R.string.listen_together_reconnecting)
ConnectionState.ERROR -> stringResource(R.string.listen_together_error)
ConnectionState.DISCONNECTED -> stringResource(R.string.listen_together_disconnected)
}
val roleText = when (role) {
RoomRole.HOST -> stringResource(R.string.listen_together_host)
RoomRole.GUEST -> stringResource(R.string.listen_together_guest)
RoomRole.NONE -> stringResource(R.string.listen_together_none)
}🤖 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 104 - 106, Replace raw enum.name usages in the InfoRow calls by
mapping ConnectionState and RoomRole to localized string resources: compute a
statusText from the connectionState (e.g., mapping ConnectionState.CONNECTED,
CONNECTING, RECONNECTING, ERROR, DISCONNECTED to R.string.listen_together_* via
stringResource) and compute a roleText from role (RoomRole.HOST, GUEST, NONE to
R.string.listen_together_*), then pass those localized strings into InfoRow
instead of connectionState.name and role.name; update the InfoRow invocations in
PlayerStatePanel (where connectionState and role are used) to use the mapped
statusText and roleText.
Comment on lines
+286
to
+289
| <string name="sleeptimer_description">Enables the sleep timer automatically with the default value by a custom time</string> | ||
| <string name="sleep_timer_repeat_description">Set a custom day and time when the sleep timer should automatically activate</string> | ||
| <string name="sleep_timer_activate">Sleep timer activates automatically when you play a song between start time and end time</string> | ||
| <string name="sleep_timer_repeat">Repeat</string> |
There was a problem hiding this comment.
Fix awkward wording in new Sleep Timer descriptions.
These strings read unnaturally in English and may confuse users.
✍️ Suggested wording update
- <string name="sleeptimer_description">Enables the sleep timer automatically with the default value by a custom time</string>
- <string name="sleep_timer_repeat_description">Set a custom day and time when the sleep timer should automatically activate</string>
- <string name="sleep_timer_activate">Sleep timer activates automatically when you play a song between start time and end time</string>
+ <string name="sleeptimer_description">Automatically enables the sleep timer with the default value at a custom time</string>
+ <string name="sleep_timer_repeat_description">Set custom days and times for when the sleep timer should automatically activate</string>
+ <string name="sleep_timer_activate">Sleep timer activates automatically when you play a song between the start and end times</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="sleeptimer_description">Enables the sleep timer automatically with the default value by a custom time</string> | |
| <string name="sleep_timer_repeat_description">Set a custom day and time when the sleep timer should automatically activate</string> | |
| <string name="sleep_timer_activate">Sleep timer activates automatically when you play a song between start time and end time</string> | |
| <string name="sleep_timer_repeat">Repeat</string> | |
| <string name="sleeptimer_description">Automatically enables the sleep timer with the default value at a custom time</string> | |
| <string name="sleep_timer_repeat_description">Set custom days and times for when the sleep timer should automatically activate</string> | |
| <string name="sleep_timer_activate">Sleep timer activates automatically when you play a song between the start and end times</string> | |
| <string name="sleep_timer_repeat">Repeat</string> |
🤖 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` around lines 286 - 289, The
four string resources have awkward English; update the values of string names
sleep_timer_description, sleep_timer_repeat_description, sleep_timer_activate,
and sleep_timer_repeat to clearer phrasing: change sleep_timer_description to
"Enable the sleep timer and set a custom duration",
sleep_timer_repeat_description to "Choose days and a time for the sleep timer to
activate automatically", sleep_timer_activate to "The sleep timer will activate
automatically for songs played between the start and end times", and keep
sleep_timer_repeat as "Repeat" (or change to "Repeat days" if clearer for UI);
edit these string values in metrolist_strings.xml accordingly.
adrielGGmotion
force-pushed
the
feature/devtools
branch
from
March 2, 2026 19:51
2cde56b to
28d66c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
39-43:⚠️ Potential issue | 🟡 MinorBuild task and APK output path are now inconsistent.
After switching to
assembleFossDebug, the install path should be updated to the correspondingfoss/debugartifact path.🛠️ Proposed doc fix
-3. Once the build is successful, you can test your changes on an emulator or a physical device. Install the generated APK located at `app/build/outputs/apk/universalFoss/debug/app-universal-foss-debug.apk` and ask a human for help testing the specific features you worked on. +3. Once the build is successful, you can test your changes on an emulator or a physical device. Install the generated APK from `app/build/outputs/apk/foss/debug/` and ask a human for help testing the specific features you worked on.🤖 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 docs reference the wrong APK path after switching the Gradle task; update the install path to match assembleFossDebug by replacing the old universalFoss artifact path with the foss debug artifact (i.e., change the listed APK from app/build/outputs/apk/universalFoss/debug/app-universal-foss-debug.apk to the corresponding assembleFossDebug output, e.g., app/build/outputs/apk/foss/debug/app-foss-debug.apk) so the install instruction aligns with the assembleFossDebug task.
♻️ Duplicate comments (6)
app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt (1)
46-46:⚠️ Potential issue | 🟠 MajorGate track-title logs behind dev-only paths (still unresolved).
Line 46, Line 54, Line 125, and Line 138 log raw
metadata.title. With in-app log capture/export, this can expose listening history unless capture/export is strictly unavailable outside debug/dev mode.🔍 Read-only verification script (tree planting + export gating)
#!/bin/bash set -euo pipefail echo "== Timber tree planting sites ==" rg -n -C3 'Timber\.plant\(|DevToolsTimberTree' --type=kotlin echo echo "== Debug/developer-mode gating signals ==" rg -n -C4 'BuildConfig\.DEBUG|developer.?mode|isDeveloperMode|dev.?mode' --type=kotlin echo echo "== Log export/capture entry points and guards ==" rg -n -C4 'export.*log|log.*export|DevToolsOverlay|LogViewer|DevTools' --type=kotlin✅ Minimal safe code change in this file
- Timber.tag(TAG).d("onSongStart: ${metadata.title}") + Timber.tag(TAG).d("onSongStart") ... - Timber.tag(TAG).d("onSongResume: ${metadata.title}") + Timber.tag(TAG).d("onSongResume") ... - Timber.tag(TAG).d("Scrobbling: ${metadata.title}") + Timber.tag(TAG).d("Scrobbling current track") ... - Timber.tag(TAG).d("Updating Now Playing: ${metadata.title}") + Timber.tag(TAG).d("Updating Now Playing")Also applies to: 54-54, 125-125, 138-138
🤖 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/ScrobbleManager.kt` at line 46, The logs in ScrobbleManager that call Timber.tag(TAG).d(...) with raw metadata.title (seen around the onSongStart logging and other log sites) can leak listening history; update those calls (all occurrences referencing metadata.title) to only log when running in a developer/debug context or to redact the title (e.g., replace with "<REDACTED>" or log a presence indicator) — use the existing debug gate (BuildConfig.DEBUG or your isDeveloperMode flag) or add one if missing, and apply the same change to the other instances that reference metadata.title to ensure consistent gating/redaction across onSongStart and the other logging sites.app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
405-408:⚠️ Potential issue | 🟠 MajorUnresolved: guard all
discordRpc.close()call sites.This is the same issue previously flagged: these close calls are still unguarded and can fail the parent coroutine/collector path.
🛠️ Hardening pattern
scope.launch(Dispatchers.IO) { - discordRpc?.close() + runCatching { discordRpc?.close() } + .onFailure { Timber.tag(TAG).e(it, "Discord RPC close failed") } }if (discordRpc?.isRpcRunning() == true) { Timber.tag(TAG).d("Discord RPC: tearing down previous instance") - discordRpc?.close() + runCatching { discordRpc?.close() } + .onFailure { Timber.tag(TAG).e(it, "Discord RPC close failed during refresh") } }scope.launch { - discordRpc?.close() + runCatching { discordRpc?.close() } + .onFailure { Timber.tag(TAG).e(it, "Discord RPC close failed on playback stop") } }Also applies to: 764-767, 2147-2150
🤖 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 405 - 408, The unguarded calls to discordRpc.close() (e.g., inside scope.launch(Dispatchers.IO) in MusicService where Timber.tag(TAG).d("Discord RPC: screen off while paused, closing connection") is logged) can throw and cancel the parent coroutine/collector; wrap every discordRpc?.close() invocation in a safe guard (e.g., check discordRpc for null and perform the close inside a try/catch or runCatching to swallow/log exceptions) so failures won’t propagate—apply the same change to the other call sites mentioned (the other discordRpc.close() occurrences in MusicService).app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt (2)
104-106:⚠️ Potential issue | 🟡 MinorLocalize listen-together enum labels before rendering.
Line [104] and Line [106] still expose
connectionState.name/role.name, which shows raw enum constants instead of localized UI text.🤖 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 104 - 106, Replace raw enum names shown in InfoRow by mapping ConnectionState and Role to localized strings before rendering: add a mapping (e.g., a when expression or extension function on the enum such as ConnectionState.toLabel(context) / Role.toLabel(context)) that returns stringResource(R.string.dev_listen_together_state_connected) etc., and pass those localized labels into InfoRow instead of connectionState.name and role.name; update the PlayerStatePanel usage so InfoRow(stringResource(R.string.dev_listen_together_status), localizedConnectionState) and InfoRow(stringResource(R.string.dev_listen_together_role), localizedRole).
171-172:⚠️ Potential issue | 🟡 MinorNormalize Media3 sentinel timeline values before formatting.
The timeline rows still render raw player times; unknown values can propagate as invalid negatives. Normalize
duration/currentPosition(e.g.,C.TIME_UNSET→ safe fallback) beforedev_timeline_format.Does `androidx.media3.common.Player.duration` return `C.TIME_UNSET` for unknown duration, and is `C.TIME_UNSET` intended to be treated as an unknown sentinel in UI formatting?Also applies to: 175-176, 182-183, 186-187, 196-197, 200-205
🤖 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 171 - 172, The timeline formatting is using raw Media3 sentinel values (e.g., Player.duration and Player.currentPosition) which can be C.TIME_UNSET; normalize those sentinel values before calling dev_timeline_format to avoid negative/invalid times. Update the uses of playerConnection.player.duration and playerConnection.player.currentPosition so that if they equal C.TIME_UNSET you substitute a safe fallback (e.g., 0 or null) or an explicit “unknown” value and then pass that normalized value into dev_timeline_format; apply the same normalization to every occurrence referenced (the lines using duration/currentPosition around the calls to dev_timeline_format). Ensure you import/qualify C.TIME_UNSET and only transform the values immediately prior to formatting, leaving the original Player API calls unchanged.app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
88-90:⚠️ Potential issue | 🟡 MinorPrune/clear
selectedLogIdswhen log rows disappear.Selections are preserved even after logs are removed, so the selected-count bar can reference non-existent rows.
🛠️ Proposed fix
+ LaunchedEffect(logs) { + val existingIds = logs.asSequence().map { it.id }.toSet() + selectedLogIds = selectedLogIds.intersect(existingIds) + } ... - SmallFloatingActionButton( - onClick = { buffer.clear() }, + SmallFloatingActionButton( + onClick = { + buffer.clear() + selectedLogIds = emptySet() + }, containerColor = MaterialTheme.colorScheme.errorContainer ) {Also applies to: 344-346
🤖 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 88 - 90, Selected log IDs (selectedLogIds / selectedLogIdsState) are not pruned when the displayed logs change, leaving selections that reference missing rows; update the selection set whenever the source log list changes by intersecting selectedLogIds with the current set of log IDs. Add a side-effect (e.g., LaunchedEffect or snapshot observer) in the LogViewerPanel that watches the logs collection and resets selectedLogIdsState.value = selectedLogIdsState.value.intersect(currentLogIds) (apply same fix where selectedLogIdsState is declared and again at the other occurrence around the 344-346 region) so selections only contain IDs that still exist.app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt (1)
121-122:⚠️ Potential issue | 🔴 CriticalGuard drag bounds before
coerceInto avoid crash on small screens.If computed min/max invert,
coerceIncan throw and crash during drag.🛠️ Proposed fix
+ val minOffsetX = minOf(-screenWidthPx + dragBoundsPaddingPx, 0f) + val maxOffsetX = maxOf(-screenWidthPx + dragBoundsPaddingPx, 0f) + val rawMinOffsetY = -screenHeightPx / 2f + dragBoundsPaddingPx + val rawMaxOffsetY = screenHeightPx / 2f - dragBoundsPaddingPx + val minOffsetY = minOf(rawMinOffsetY, rawMaxOffsetY) + val maxOffsetY = maxOf(rawMinOffsetY, rawMaxOffsetY) detectDragGestures { change, dragAmount -> change.consume() - offsetX = (offsetX + dragAmount.x).coerceIn(-screenWidthPx + dragBoundsPaddingPx, 0f) - offsetY = (offsetY + dragAmount.y).coerceIn(-screenHeightPx / 2f + dragBoundsPaddingPx, screenHeightPx / 2f - dragBoundsPaddingPx) + offsetX = (offsetX + dragAmount.x).coerceIn(minOffsetX, maxOffsetX) + offsetY = (offsetY + dragAmount.y).coerceIn(minOffsetY, maxOffsetY) }In Kotlin stdlib, what happens when `coerceIn(min, max)` is called with `min > max`?🤖 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 121 - 122, The current coerceIn calls can throw if the computed min > max on very small screens; before calling coerceIn for offsetX and offsetY compute and sanitize bounds (e.g., val minX = (-screenWidthPx + dragBoundsPaddingPx), maxX = 0f; if (minX > maxX) { val fixed = (minX + maxX) / 2f; minX = fixed; maxX = fixed } and similarly for minY = (-screenHeightPx / 2f + dragBoundsPaddingPx) and maxY = (screenHeightPx / 2f - dragBoundsPaddingPx)), then call offsetX = (offsetX + dragAmount.x).coerceIn(minX, maxX) and offsetY = (offsetY + dragAmount.y).coerceIn(minY, maxY); this ensures coerceIn(min, max) is never invoked with min > max and prevents crashes.
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
120-123: NarrowmaskTokenvisibility to internal use.This helper is only used inside
DiscordRPC; keeping it private reduces accidental external coupling.♻️ Proposed refactor
- fun maskToken(token: String): String { + private fun maskToken(token: String): String { if (token.length <= 8) return "****" return "${token.take(4)}...${token.takeLast(2)}" }🤖 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 120 - 123, The helper function maskToken is publicly visible but only used inside DiscordRPC; make its visibility private to prevent external coupling by changing its declaration to a private function (e.g., private fun maskToken(token: String): String) or move it inside the DiscordRPC class/object scope if currently top-level; update any references inside DiscordRPC to continue using maskToken and ensure no external callers rely on it before making it private.app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
254-254: Prefer read-only exposure for internal service state.These properties are now mutable across the module. If DevTools only needs to inspect state, keep external access read-only to avoid accidental state mutation.
♻️ Suggested tightening
- internal var crossfadeEnabled = false + internal var crossfadeEnabled = false + private set ... - internal var isAudioEffectSessionOpened = false - internal var loudnessEnhancer: LoudnessEnhancer? = null + internal var isAudioEffectSessionOpened = false + private set + internal var loudnessEnhancer: LoudnessEnhancer? = null + private set ... - internal var discordRpc: DiscordRPC? = null + internal var discordRpc: DiscordRPC? = null + private set ... - internal var scrobbleManager: ScrobbleManager? = null + internal var scrobbleManager: ScrobbleManager? = null + private setAlso applies to: 362-363, 365-365, 369-369
🤖 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` at line 254, The mutable service state properties (e.g., MusicService.internal var crossfadeEnabled) should be exposed read-only so DevTools can inspect but not mutate them; change these module-visible mutable vars to either private vars with public val getters or keep them as vars but add private set (or a private backing field + public val) so external code cannot write to them; apply the same change for the other mutable properties referenced in the review (the ones at the noted locations) to tighten access and prevent accidental mutation while preserving internal mutability inside MusicService.
🤖 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/ActionsPanel.kt`:
- Around line 162-167: The cache-clear code computes pre-delete size and then
calls deleteRecursively() on each file but ignores its boolean result, which can
show a success Toast even if deletions failed; modify the Dispatchers.IO block
in ActionsPanel.kt to attempt deletion per file (use
context.cacheDir.listFiles()?.forEach or walkTopDown().forEach),
collect/deleteRecursively() return values or recompute the directory size after
deletion (using context.cacheDir.walkTopDown().filter { it.isFile }.map {
it.length() }.sum()) to detect failures, and then return either the freed MB or
an error flag so the UI can show a success Toast with the actual freed size
(sizeMb) or a failure Toast when any deleteRecursively() returned false.
- Around line 49-62: The replacement uses "$1=<REDACTED>" in redactSensitiveData
but several regexes in SENSITIVE_PATTERNS (visitorData, dataSyncId, SAPISID,
__Secure-[A-Z]+ and similar) have no capture group 1, causing
IndexOutOfBoundsException at runtime; fix by ensuring every Pattern in
SENSITIVE_PATTERNS captures the key/name as group 1 (e.g., wrap the left-hand
token portion in (...) so $1 is defined) or alternatively change
redactSensitiveData to perform a safe replacement that does not assume group 1
(e.g., rebuild the replacement using the matcher’s groups). Update
SENSITIVE_PATTERNS and/or redactSensitiveData accordingly so every match
provides the expected group 1 before using "$1=<REDACTED>".
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 57-58: The calculation for remainingDuration can go negative when
currentPlaybackTimeMillis exceeds song.song.duration, producing a past endTime
for presence; clamp the computed remainingDuration to a non-negative value
before adjusting for playback speed (i.e., compute remainingDuration = max(0,
song.song.duration * 1000L - currentPlaybackTimeMillis) and then compute
adjustedRemainingDuration = (remainingDuration / validPlaybackSpeed).toLong()),
or alternatively clamp adjustedRemainingDuration to >= 0 before using it to
derive endTime/presence; update the logic around remainingDuration,
adjustedRemainingDuration, and any endTime/presence usage to ensure no negative
timestamps are produced.
In `@development_guide.md`:
- Around line 23-24: Update the APK output path text to reflect the current
single-dimension flavor configuration: change the ls path reference from
"app/build/outputs/apk/universalFoss/debug/app-universal-foss-debug.apk" to
"app/build/outputs/apk/foss/debug/app-foss-debug.apk" wherever it appears (the
example block in development_guide.md that follows the :app:assembleFossDebug
task and the corresponding reference in AGENTS.md); ensure the assemble task
reference (:app:assembleFossDebug) remains unchanged and that the new path uses
the "foss" directory and "app-foss-debug.apk" filename.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 39-43: The docs reference the wrong APK path after switching the
Gradle task; update the install path to match assembleFossDebug by replacing the
old universalFoss artifact path with the foss debug artifact (i.e., change the
listed APK from
app/build/outputs/apk/universalFoss/debug/app-universal-foss-debug.apk to the
corresponding assembleFossDebug output, e.g.,
app/build/outputs/apk/foss/debug/app-foss-debug.apk) so the install instruction
aligns with the assembleFossDebug task.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`:
- Around line 121-122: The current coerceIn calls can throw if the computed min
> max on very small screens; before calling coerceIn for offsetX and offsetY
compute and sanitize bounds (e.g., val minX = (-screenWidthPx +
dragBoundsPaddingPx), maxX = 0f; if (minX > maxX) { val fixed = (minX + maxX) /
2f; minX = fixed; maxX = fixed } and similarly for minY = (-screenHeightPx / 2f
+ dragBoundsPaddingPx) and maxY = (screenHeightPx / 2f - dragBoundsPaddingPx)),
then call offsetX = (offsetX + dragAmount.x).coerceIn(minX, maxX) and offsetY =
(offsetY + dragAmount.y).coerceIn(minY, maxY); this ensures coerceIn(min, max)
is never invoked with min > max and prevents crashes.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 88-90: Selected log IDs (selectedLogIds / selectedLogIdsState) are
not pruned when the displayed logs change, leaving selections that reference
missing rows; update the selection set whenever the source log list changes by
intersecting selectedLogIds with the current set of log IDs. Add a side-effect
(e.g., LaunchedEffect or snapshot observer) in the LogViewerPanel that watches
the logs collection and resets selectedLogIdsState.value =
selectedLogIdsState.value.intersect(currentLogIds) (apply same fix where
selectedLogIdsState is declared and again at the other occurrence around the
344-346 region) so selections only contain IDs that still exist.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt`:
- Around line 104-106: Replace raw enum names shown in InfoRow by mapping
ConnectionState and Role to localized strings before rendering: add a mapping
(e.g., a when expression or extension function on the enum such as
ConnectionState.toLabel(context) / Role.toLabel(context)) that returns
stringResource(R.string.dev_listen_together_state_connected) etc., and pass
those localized labels into InfoRow instead of connectionState.name and
role.name; update the PlayerStatePanel usage so
InfoRow(stringResource(R.string.dev_listen_together_status),
localizedConnectionState) and
InfoRow(stringResource(R.string.dev_listen_together_role), localizedRole).
- Around line 171-172: The timeline formatting is using raw Media3 sentinel
values (e.g., Player.duration and Player.currentPosition) which can be
C.TIME_UNSET; normalize those sentinel values before calling dev_timeline_format
to avoid negative/invalid times. Update the uses of
playerConnection.player.duration and playerConnection.player.currentPosition so
that if they equal C.TIME_UNSET you substitute a safe fallback (e.g., 0 or null)
or an explicit “unknown” value and then pass that normalized value into
dev_timeline_format; apply the same normalization to every occurrence referenced
(the lines using duration/currentPosition around the calls to
dev_timeline_format). Ensure you import/qualify C.TIME_UNSET and only transform
the values immediately prior to formatting, leaving the original Player API
calls unchanged.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 405-408: The unguarded calls to discordRpc.close() (e.g., inside
scope.launch(Dispatchers.IO) in MusicService where Timber.tag(TAG).d("Discord
RPC: screen off while paused, closing connection") is logged) can throw and
cancel the parent coroutine/collector; wrap every discordRpc?.close() invocation
in a safe guard (e.g., check discordRpc for null and perform the close inside a
try/catch or runCatching to swallow/log exceptions) so failures won’t
propagate—apply the same change to the other call sites mentioned (the other
discordRpc.close() occurrences in MusicService).
In `@app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt`:
- Line 46: The logs in ScrobbleManager that call Timber.tag(TAG).d(...) with raw
metadata.title (seen around the onSongStart logging and other log sites) can
leak listening history; update those calls (all occurrences referencing
metadata.title) to only log when running in a developer/debug context or to
redact the title (e.g., replace with "<REDACTED>" or log a presence indicator) —
use the existing debug gate (BuildConfig.DEBUG or your isDeveloperMode flag) or
add one if missing, and apply the same change to the other instances that
reference metadata.title to ensure consistent gating/redaction across
onSongStart and the other logging sites.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Line 254: The mutable service state properties (e.g., MusicService.internal
var crossfadeEnabled) should be exposed read-only so DevTools can inspect but
not mutate them; change these module-visible mutable vars to either private vars
with public val getters or keep them as vars but add private set (or a private
backing field + public val) so external code cannot write to them; apply the
same change for the other mutable properties referenced in the review (the ones
at the noted locations) to tighten access and prevent accidental mutation while
preserving internal mutability inside MusicService.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 120-123: The helper function maskToken is publicly visible but
only used inside DiscordRPC; make its visibility private to prevent external
coupling by changing its declaration to a private function (e.g., private fun
maskToken(token: String): String) or move it inside the DiscordRPC class/object
scope if currently top-level; update any references inside DiscordRPC to
continue using maskToken and ensure no external callers rely on it before making
it private.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
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/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/component/SleepTimerDialog.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/PlayerSettings.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
🚧 Files skipped from review as they are similar to previous changes (11)
- app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt
- app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt
- app/src/main/kotlin/com/metrolist/music/utils/Utils.kt
- app/src/main/res/values/values.xml
- app/src/main/kotlin/com/metrolist/music/di/AppModule.kt
- app/src/main/kotlin/com/metrolist/music/ui/screens/settings/SettingsScreen.kt
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsTimberTree.kt
- app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt
- app/src/main/kotlin/com/metrolist/music/db/DatabaseDao.kt
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLog.kt
- app/src/main/kotlin/com/metrolist/music/ui/component/SleepTimerDialog.kt
Comment on lines
+162
to
+167
| val sizeMb = withContext(Dispatchers.IO) { | ||
| val size = context.cacheDir.walkTopDown().filter { it.isFile }.map { it.length() }.sum() | ||
| context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } | ||
| size / 1024 / 1024 | ||
| } | ||
| Toast.makeText(context, context.getString(R.string.cleared_cache_mb, sizeMb), Toast.LENGTH_SHORT).show() |
There was a problem hiding this comment.
Cache clear may report success even when deletion fails.
deleteRecursively() returns false on failure, but that signal is ignored, so the UI can show a successful clear with leftover files.
🔧 Proposed fix
val sizeMb = withContext(Dispatchers.IO) {
val size = context.cacheDir.walkTopDown().filter { it.isFile }.map { it.length() }.sum()
- context.cacheDir.listFiles()?.forEach { it.deleteRecursively() }
+ val failed = context.cacheDir.listFiles()
+ ?.filterNot { it.deleteRecursively() }
+ .orEmpty()
+ if (failed.isNotEmpty()) {
+ throw IllegalStateException("Failed to delete ${failed.size} cache entries")
+ }
size / 1024 / 1024
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val sizeMb = withContext(Dispatchers.IO) { | |
| val size = context.cacheDir.walkTopDown().filter { it.isFile }.map { it.length() }.sum() | |
| context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } | |
| size / 1024 / 1024 | |
| } | |
| Toast.makeText(context, context.getString(R.string.cleared_cache_mb, sizeMb), Toast.LENGTH_SHORT).show() | |
| val sizeMb = withContext(Dispatchers.IO) { | |
| val size = context.cacheDir.walkTopDown().filter { it.isFile }.map { it.length() }.sum() | |
| val failed = context.cacheDir.listFiles() | |
| ?.filterNot { it.deleteRecursively() } | |
| .orEmpty() | |
| if (failed.isNotEmpty()) { | |
| throw IllegalStateException("Failed to delete ${failed.size} cache entries") | |
| } | |
| size / 1024 / 1024 | |
| } | |
| Toast.makeText(context, context.getString(R.string.cleared_cache_mb, sizeMb), 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 162 - 167, The cache-clear code computes pre-delete size and then calls
deleteRecursively() on each file but ignores its boolean result, which can show
a success Toast even if deletions failed; modify the Dispatchers.IO block in
ActionsPanel.kt to attempt deletion per file (use
context.cacheDir.listFiles()?.forEach or walkTopDown().forEach),
collect/deleteRecursively() return values or recompute the directory size after
deletion (using context.cacheDir.walkTopDown().filter { it.isFile }.map {
it.length() }.sum()) to detect failures, and then return either the freed MB or
an error flag so the UI can show a success Toast with the actual freed size
(sizeMb) or a failure Toast when any deleteRecursively() returned false.
Comment on lines
+23
to
24
| ./gradlew :app:assembleFossDebug | ||
| ls app/build/outputs/apk/universalFoss/debug/app-universal-foss-debug.apk |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the development_guide.md file
find . -name "development_guide.md" -type fRepository: adrielGGmotion/Metrolist
Length of output: 90
🏁 Script executed:
# Look for Gradle build files
find . -name "build.gradle*" -type f | head -20Repository: adrielGGmotion/Metrolist
Length of output: 330
🏁 Script executed:
# Check repository structure
ls -laRepository: adrielGGmotion/Metrolist
Length of output: 2121
🏁 Script executed:
# Read development_guide.md to see the current state
cat -n development_guide.mdRepository: adrielGGmotion/Metrolist
Length of output: 1826
🏁 Script executed:
# Read the app build.gradle.kts to understand Gradle task configuration
wc -l app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 92
🏁 Script executed:
# Look for flavor and variant definitions
head -150 app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 4703
🏁 Script executed:
# Search for bundle, split, or APK output configuration in build.gradle.kts
grep -n -i "bundle\|split\|apk\|output\|architecture" app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 207
🏁 Script executed:
# Search for any references to "universalFoss" or "universal" in the codebase
rg -i "universalFoss|universalGms" --type markdown --type gradleRepository: adrielGGmotion/Metrolist
Length of output: 448
🏁 Script executed:
# Look at the rest of the build.gradle.kts file to see if there are bundle or output configurations
tail -100 app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 3002
🏁 Script executed:
# Search for any bundle or output path customization in the entire build.gradle.kts
cat app/build.gradle.kts | grep -A 20 -B 5 "bundle\|outputs\|archivesBaseName\|apkName"Repository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Check if there are any gradle/Kotlin files that might customize the APK output path or name
rg -i "universalFoss|archivesBaseName|apkName|output.apk" --type gradleRepository: adrielGGmotion/Metrolist
Length of output: 216
🏁 Script executed:
# Search for any gradle tasks or conventions that might be customizing output
rg -i "tasks\." app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 149
🏁 Script executed:
# Check if there's a Bundle configuration or if the project uses dynamic feature modules
rg "bundle\|splits\|dynamicFeatures" app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Search all gradle files for any custom conventions or plugins that might affect output
find . -name "*.gradle.kts" -o -name "*.gradle" | xargs grep -l "universalFoss\|archivesBaseName\|apk"Repository: adrielGGmotion/Metrolist
Length of output: 52
🏁 Script executed:
# Check if there's a buildTypes configuration that might affect APK naming
sed -n '70,110p' app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 1342
🏁 Script executed:
# Look for any settings in gradle.properties that might affect build output
cat gradle.propertiesRepository: adrielGGmotion/Metrolist
Length of output: 1606
🏁 Script executed:
# Check the entire app/build.gradle.kts for any splits or bundleConfig
cat app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 7298
🏁 Script executed:
# Check if there's a convention plugin or other gradle file that might be configuring output paths
find . -type f \( -name "*.gradle.kts" -o -name "*.gradle" \) -exec grep -l "universalFoss\|splits\|bundle" {} \;Repository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Check buildTypes and productFlavors more carefully for output configuration
grep -A 30 "productFlavors\|buildTypes" app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 2087
🏁 Script executed:
# Check if there are any bundle or ABI splits configured that might create universalFoss path
rg -i "bundle|split|abi" app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 99
🏁 Script executed:
# Look at AGENTS.md to see if it provides any additional context
cat -n AGENTS.mdRepository: adrielGGmotion/Metrolist
Length of output: 4034
🏁 Script executed:
# Search the entire codebase for any gradle convention plugins or other build configuration
find . -name "gradle" -type dRepository: adrielGGmotion/Metrolist
Length of output: 76
🏁 Script executed:
# Check if there's a buildSrc directory with gradle conventions
ls -la buildSrc 2>/dev/null || echo "No buildSrc directory"Repository: adrielGGmotion/Metrolist
Length of output: 89
🏁 Script executed:
# Check gradle wrapper version to understand AGP behavior
cat gradle/wrapper/gradle-wrapper.propertiesRepository: adrielGGmotion/Metrolist
Length of output: 319
🏁 Script executed:
# Search for any build.gradle files at the root or gradle folder that might define conventions
find gradle -type f -name "*.gradle*" 2>/dev/nullRepository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Look for any build configuration or convention files in gradle directory
ls -la gradle/Repository: adrielGGmotion/Metrolist
Length of output: 387
🏁 Script executed:
# Search the entire codebase for any references to "universalFoss" - could be a custom task or configuration
rg -i "universalFoss" --type-listRepository: adrielGGmotion/Metrolist
Length of output: 457
🏁 Script executed:
# Search for "universalFoss" in the entire codebase
rg "universalFoss"Repository: adrielGGmotion/Metrolist
Length of output: 448
🏁 Script executed:
# Check Android Gradle Plugin documentation - with AGP 9.x, APK output path might have changed
# Let's search for any gradle task configuration that might create the universalFoss path
rg "apk|outputs" app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Check if there's a splits configuration or bundle release configuration
grep -n "splits\|bundle" app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Search for any .gradle or .kts files that might configure splits or ABI separation
find . -type f \( -name "*.gradle.kts" -o -name "*.gradle" \) | xargs grep -l "split\|bundle\|abi" 2>/dev/nullRepository: adrielGGmotion/Metrolist
Length of output: 52
🏁 Script executed:
# Check git history to see if there was a change from assembleuniversalFossDebug to assembleFossDebug
git log --oneline -20 -- development_guide.md AGENTS.md 2>/dev/null | head -20Repository: adrielGGmotion/Metrolist
Length of output: 184
🏁 Script executed:
# Look for any android variant or bundle configurations that might explain the universalFoss path
grep -rn "universalFoss\|universal" . --include="*.gradle*" --include="*.kts"Repository: adrielGGmotion/Metrolist
Length of output: 161
🏁 Script executed:
# Check the actual diff of the commit that changed the dev guide
git show 28d66c05 -- development_guide.md AGENTS.md 2>/dev/null | head -100Repository: adrielGGmotion/Metrolist
Length of output: 5840
🏁 Script executed:
# Look for any variant-specific output configuration in the app module
find app -type f -name "*.gradle*" -o -name "*.kts" | xargs cat 2>/dev/null | grep -A 5 -B 5 "universalFoss\|variant\|splits"Repository: adrielGGmotion/Metrolist
Length of output: 839
🏁 Script executed:
# Check if AGP version in libs.versions.toml might affect output paths
cat gradle/libs.versions.toml | grep -i "agp\|androidGradlePlugin\|android"Repository: adrielGGmotion/Metrolist
Length of output: 2714
🏁 Script executed:
# Check if there are bundle or splits configurations I might have missed
grep -n "bundle\|splits\|abi\|variant" app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 354
🏁 Script executed:
# Search git history for any changes to build.gradle.kts related to APK output
git log --oneline app/build.gradle.kts 2>/dev/null | head -10Repository: adrielGGmotion/Metrolist
Length of output: 184
🏁 Script executed:
# Look at the git diff of the last commit to app/build.gradle.kts to understand the configuration
git show HEAD:app/build.gradle.kts 2>/dev/null | grep -A 10 -B 10 "splits\|bundle\|variant"Repository: adrielGGmotion/Metrolist
Length of output: 1320
🏁 Script executed:
# Check if the APK path might be using the flavor name differently - search for any custom variants
rg "foss|gms|variant" app/build.gradle.kts -A 3 -B 1Repository: adrielGGmotion/Metrolist
Length of output: 899
🏁 Script executed:
# Search for any Android Gradle Plugin version-specific behavior or custom apk naming
rg "baseName|archivesBaseName|apkName\|getVersionName" . --type gradleRepository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Check if there are any gradle tasks defined that might create the universalFoss path
rg "tasks\.|android\." app/build.gradle.ktsRepository: adrielGGmotion/Metrolist
Length of output: 183
🏁 Script executed:
# Look for Bundle Release configurations that might affect APK output
rg "bundle|bundleRelease|releaseBundle" . --type gradleRepository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Check if the project uses any build flavor that might be hidden
git show HEAD:app/build.gradle.kts 2>/dev/null | head -100Repository: adrielGGmotion/Metrolist
Length of output: 3420
🏁 Script executed:
# Check git log to see if build.gradle.kts had different configuration before
git log --all --oneline app/build.gradle.kts 2>/dev/nullRepository: adrielGGmotion/Metrolist
Length of output: 5871
🏁 Script executed:
# Search for any commit that might have introduced or removed split configurations
git log -p --all -- app/build.gradle.kts 2>/dev/null | grep -A 5 -B 5 "splits\|bundle\|universalFoss" | head -50Repository: adrielGGmotion/Metrolist
Length of output: 50
🏁 Script executed:
# Check if there's a previous version of build.gradle that had splits
git log --all --oneline -- "app/build.gradle*" 2>/dev/nullRepository: adrielGGmotion/Metrolist
Length of output: 5871
🏁 Script executed:
# Look for any environment-specific configuration or CI/CD files that might affect the build
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "assembleFossDebug\|apk\|outputs" 2>/dev/nullRepository: adrielGGmotion/Metrolist
Length of output: 238
🏁 Script executed:
# Look at the commit that removed ABI flavor variants
git show 0c1ea63b -- app/build.gradle.kts | head -150Repository: adrielGGmotion/Metrolist
Length of output: 2017
🏁 Script executed:
# Check what the build.gradle looked like before the consolidation
git show 0c1ea63b^:app/build.gradle.kts | grep -A 20 -B 5 "split\|bundle\|abi\|universal\|variant" | head -60Repository: adrielGGmotion/Metrolist
Length of output: 2063
🏁 Script executed:
# Check the revert commit to see what was restored
git show 076c4f4f -- app/build.gradle.kts | head -100Repository: adrielGGmotion/Metrolist
Length of output: 1413
🏁 Script executed:
# Look at what commit 5a64ea57 changed (the one that was reverted by 076c4f4f)
git show 5a64ea57:app/build.gradle.kts | grep -A 30 "flavorDimensions\|productFlavors"Repository: adrielGGmotion/Metrolist
Length of output: 1187
🏁 Script executed:
# Check what the build output path would actually be with current configuration
# Search for any gradle output configuration or task naming customization
grep -rn "apk\|outputs\|variant" . --include="*.gradle.kts" --include="*.gradle"Repository: adrielGGmotion/Metrolist
Length of output: 469
Update the APK output path to match the current single-dimension flavor configuration.
The task :app:assembleFossDebug builds with only the "variant" flavor dimension (foss/gms), as the ABI flavor dimension was consolidated into a universal build in commit 0c1ea63. The output path should be app/build/outputs/apk/foss/debug/ (not universalFoss/debug/). Update line 24 and the corresponding reference in AGENTS.md to:
ls app/build/outputs/apk/foss/debug/app-foss-debug.apk🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@development_guide.md` around lines 23 - 24, Update the APK output path text
to reflect the current single-dimension flavor configuration: change the ls path
reference from
"app/build/outputs/apk/universalFoss/debug/app-universal-foss-debug.apk" to
"app/build/outputs/apk/foss/debug/app-foss-debug.apk" wherever it appears (the
example block in development_guide.md that follows the :app:assembleFossDebug
task and the corresponding reference in AGENTS.md); ensure the assemble task
reference (:app:assembleFossDebug) remains unchanged and that the new path uses
the "foss" directory and "app-foss-debug.apk" filename.
adrielGGmotion
force-pushed
the
feature/devtools
branch
from
March 2, 2026 20:19
28d66c0 to
65b2b68
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt (1)
46-46:⚠️ Potential issue | 🟠 MajorGate track-title logs to dev-only paths and avoid raw titles.
Line 46, Line 54, Line 125, and Line 138 still log
metadata.titledirectly. With in-app log capture/export, this can expose listening history.🔧 Suggested hardening
+import com.metrolist.music.BuildConfig ... class ScrobbleManager( @@ ) { @@ + private inline fun devLog(message: () -> String) { + if (BuildConfig.DEBUG) Timber.tag(TAG).d(message()) + } @@ - Timber.tag(TAG).d("onSongStart: ${metadata.title}") + devLog { "onSongStart" } @@ - Timber.tag(TAG).d("onSongResume: ${metadata.title}") + devLog { "onSongResume" } @@ - Timber.tag(TAG).d("Scrobbling: ${metadata.title}") + devLog { "Scrobbling current track" } @@ - Timber.tag(TAG).d("Updating Now Playing: ${metadata.title}") + devLog { "Updating Now Playing" }Also applies to: 54-54, 125-125, 138-138
🤖 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/ScrobbleManager.kt` at line 46, The logs in ScrobbleManager (calls like onSongStart and the other places logging metadata.title) expose raw track titles; update those Timber.tag(...) calls to only run on dev builds (guard with BuildConfig.DEBUG or a isDebuggable check) and never log the plain metadata.title — instead log a non-identifying placeholder or a deterministic hash/obfuscated value (e.g., SHA-256 of metadata.title) so you can correlate events in dev without exposing raw listening history; apply this change to the logging sites in ScrobbleManager (the onSongStart and the other methods referencing metadata.title).app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
57-58:⚠️ Potential issue | 🟠 MajorClamp remaining duration to prevent invalid past
endTime.At Line 57–58,
remainingDurationcan go negative when playback position exceeds track duration, producing invalid timestamps for presence.🛠️ Suggested fix
- val remainingDuration = song.song.duration * 1000L - currentPlaybackTimeMillis + val remainingDuration = + (song.song.duration * 1000L - currentPlaybackTimeMillis).coerceAtLeast(0L) val adjustedRemainingDuration = (remainingDuration / validPlaybackSpeed).toLong()🤖 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 57 - 58, remainingDuration can be negative causing invalid presence endTime; clamp it to zero before adjusting for playback speed. In the DiscordRPC code compute remainingDuration = max(0, song.song.duration * 1000L - currentPlaybackTimeMillis) and then compute adjustedRemainingDuration = (remainingDuration / validPlaybackSpeed).toLong(), ensuring you reference the existing remainingDuration and adjustedRemainingDuration variables so the presence endTime is never set to a past timestamp.app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
405-408:⚠️ Potential issue | 🟠 MajorGuard all
discordRpc?.close()call sites; avoid uncaught coroutine cancellation.Line 407, Line 766, and Line 2149 still call
close()withoutrunCatching. A thrown exception here can cancel service coroutines and interrupt unrelated flows/jobs.🛡️ Suggested consolidation
+ private fun closeDiscordRpcAsync(reason: String) { + val rpc = discordRpc + discordRpc = null + scope.launch(Dispatchers.IO) { + runCatching { rpc?.close() } + .onFailure { Timber.tag(TAG).e(it, "Discord RPC close failed: %s", reason) } + } + } @@ - scope.launch(Dispatchers.IO) { - discordRpc?.close() - } + closeDiscordRpcAsync("screen_off_paused") @@ - discordRpc?.close() + closeDiscordRpcAsync("token_refresh") @@ - scope.launch { - discordRpc?.close() - } + closeDiscordRpcAsync("playback_stopped")Also applies to: 764-767, 2147-2150
🤖 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 405 - 408, Several call sites directly invoke discordRpc?.close() inside coroutines (e.g., within scope.launch(Dispatchers.IO) and other shutdown paths) which can throw and cancel surrounding coroutines; wrap each discordRpc?.close() call in a non-throwing guard such as runCatching { discordRpc?.close() }.onFailure { Timber.e(it, "Error closing Discord RPC") } (or an equivalent try/catch that logs the Throwable and does not rethrow), and apply this change to every place that calls discordRpc?.close() (including the calls currently inside scope.launch and any lifecycle/shutdown methods) so exceptions are logged but do not cancel service coroutines.app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt (2)
104-106:⚠️ Potential issue | 🟡 MinorAvoid raw enum
.namein UI; map to localized strings.Line 104 and Line 106 still use
connectionState.name/role.name, which bypass localization and exposes internal enum labels.🤖 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 104 - 106, The UI is displaying raw enum .name for connectionState and role (used in InfoRow calls) which bypasses localization; replace those uses with localized strings by mapping each enum value to a string resource (either via a when expression in PlayerStatePanel or small extension functions like ConnectionState.toLocalizedString(context) and Role.toLocalizedString(context)) and pass stringResource(R.<...>) results to InfoRow instead of connectionState.name and role.name, ensuring each enum branch maps to the appropriate R.string resource for localization.
171-172:⚠️ Potential issue | 🟡 MinorNormalize Media3 unknown-time sentinels before formatting timeline values.
At Line 171/175/186 and Line 182/196, raw player times are used directly; Line 205 formats them as-is.
C.TIME_UNSET(and other negative sentinel cases) should be normalized before display.🛠️ Suggested fix
+import androidx.media3.common.C @@ `@Composable` fun TimelineRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { + fun normalizeTime(value: Long): Long = if (value == C.TIME_UNSET || value < 0L) 0L else value @@ - duration = playerConnection.player.duration + duration = normalizeTime(playerConnection.player.duration) @@ - duration = playerConnection.player.duration + duration = normalizeTime(playerConnection.player.duration) @@ - currentPosition = playerConnection.player.currentPosition + currentPosition = normalizeTime(playerConnection.player.currentPosition) @@ - currentPosition = playerConnection.player.currentPosition - duration = playerConnection.player.duration + currentPosition = normalizeTime(playerConnection.player.currentPosition) + duration = normalizeTime(playerConnection.player.duration) @@ - currentPosition = playerConnection.player.currentPosition + currentPosition = normalizeTime(playerConnection.player.currentPosition) @@ - currentPosition = playerConnection.player.currentPosition + currentPosition = normalizeTime(playerConnection.player.currentPosition)#!/bin/bash # Verify raw Media3 time fields are normalized before rendering. rg -n 'playerConnection\.player\.(duration|currentPosition|bufferedPosition)|C\.TIME_UNSET' app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt -C 2Also applies to: 175-176, 182-183, 186-187, 196-197, 205-205
app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
88-90:⚠️ Potential issue | 🟡 MinorPrune selection state when logs are cleared or rotated.
selectedLogIdscan outlive backing rows (clear and ring-buffer eviction), causing stale selected counts.🧹 Suggested fix
@@ var selectedLogIds by selectedLogIdsState @@ + LaunchedEffect(logs) { + val existing = logs.asSequence().map { it.id }.toSet() + selectedLogIds = selectedLogIds.intersect(existing) + } @@ - SmallFloatingActionButton( - onClick = { buffer.clear() }, + SmallFloatingActionButton( + onClick = { + buffer.clear() + selectedLogIds = emptySet() + }, containerColor = MaterialTheme.colorScheme.errorContainer ) {Also applies to: 344-346
🤖 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 88 - 90, selectedLogIds can retain IDs that no longer exist after log clears or ring-buffer rotations; update the selection whenever the backing log list changes by pruning selectedLogIds to the intersection of current log IDs (or clearing it when logs are emptied). In LogViewerPanel, add a derived effect or observe the logs collection change (where selectedLogIds is declared and around the other occurrence at lines ~344-346) and replace selectedLogIds with selectedLogIds.filter { it in currentLogIds } (or set to emptySet() if currentLogIds is empty) so the selected count always reflects existing rows.app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (1)
139-144:⚠️ Potential issue | 🟡 MinorCache clear may report success even when deletion fails.
deleteRecursively()returnsfalseon failure, but that result is ignored. The UI shows a success toast with the pre-deletion size even if some files couldn't be deleted.🔧 Proposed fix: verify deletion success
val sizeMb = withContext(Dispatchers.IO) { val size = context.cacheDir.walkTopDown().filter { it.isFile }.map { it.length() }.sum() - context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } - size / 1024 / 1024 + val allDeleted = context.cacheDir.listFiles() + ?.all { it.deleteRecursively() } ?: true + Pair(size / 1024 / 1024, allDeleted) } - Toast.makeText(context, context.getString(R.string.cleared_cache_mb, sizeMb), Toast.LENGTH_SHORT).show() + val (freedMb, success) = sizeMb + if (success) { + Toast.makeText(context, context.getString(R.string.cleared_cache_mb, freedMb), Toast.LENGTH_SHORT).show() + } else { + Toast.makeText(context, context.getString(R.string.partial_cache_clear, freedMb), 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 139 - 144, The cache-clear path computes pre-deletion size (sizeMb) but ignores deleteRecursively() results, so the UI can show a success toast even if deletion failed; update the block in ActionsPanel.kt that computes sizeMb and performs deletion to capture each deletion result (e.g., collect boolean returns from context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } or use map/filter to determine any failures), recompute or verify post-deletion size (or check aggregated boolean success) and only show the success Toast when deletions succeeded, otherwise show a failure/error Toast with appropriate message; reference the sizeMb variable and the deleteRecursively() calls when making the changes.app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt (1)
119-123:⚠️ Potential issue | 🔴 CriticalGuard drag bounds against inversion to prevent runtime crashes.
The Y-axis drag bounds can invert on small devices when
2 × dragBoundsPaddingPx > screenHeightPx. Kotlin'scoerceIn()throwsIllegalArgumentExceptionwhen given an inverted range (min > max), causing an app crash during drag gestures.🛠️ Proposed fix: normalize bounds before calling coerceIn
val screenWidthPx = with(density) { configuration.screenWidthDp.dp.toPx() } val screenHeightPx = with(density) { configuration.screenHeightDp.dp.toPx() } val context = androidx.compose.ui.platform.LocalContext.current val dragBoundsPaddingPx = with(density) { context.resources.getDimension(R.dimen.devtools_drag_bounds_padding).toDp().toPx() } + + val minOffsetX = -screenWidthPx + dragBoundsPaddingPx + val maxOffsetX = 0f + val rawMinOffsetY = -screenHeightPx / 2f + dragBoundsPaddingPx + val rawMaxOffsetY = screenHeightPx / 2f - dragBoundsPaddingPx + // Guard against inverted range on small screens + val minOffsetY = minOf(rawMinOffsetY, rawMaxOffsetY) + val maxOffsetY = maxOf(rawMinOffsetY, rawMaxOffsetY) detectDragGestures { change, dragAmount -> change.consume() - offsetX = (offsetX + dragAmount.x).coerceIn(-screenWidthPx + dragBoundsPaddingPx, 0f) - offsetY = (offsetY + dragAmount.y).coerceIn(-screenHeightPx / 2f + dragBoundsPaddingPx, screenHeightPx / 2f - dragBoundsPaddingPx) + offsetX = (offsetX + dragAmount.x).coerceIn(minOffsetX, maxOffsetX) + offsetY = (offsetY + dragAmount.y).coerceIn(minOffsetY, maxOffsetY) }🤖 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 119 - 123, The Y-axis bounds passed to coerceIn can be inverted when 2 * dragBoundsPaddingPx > screenHeightPx, causing coerceIn to throw; before calling coerceIn for offsetY in the detectDragGestures lambda, compute normalized minY and maxY (e.g., calculate rawMin = -screenHeightPx / 2f + dragBoundsPaddingPx and rawMax = screenHeightPx / 2f - dragBoundsPaddingPx, then if rawMin > rawMax set both to a safe fallback like the midpoint or the smaller value) and call offsetY = (offsetY + dragAmount.y).coerceIn(minY, maxY) using those normalized bounds to prevent IllegalArgumentException while keeping the same variables (detectDragGestures, offsetY, dragBoundsPaddingPx, screenHeightPx, coerceIn).
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
254-254: Expose internal state as read-only where possible (private set).These fields were widened to
internal var; this allows writes from anywhere in-module and can break service invariants unintentionally. If DevTools only needs reads, keep setters private.✂️ Suggested tightening
- internal var crossfadeEnabled = false + internal var crossfadeEnabled = false + private set @@ - internal var isAudioEffectSessionOpened = false - internal var loudnessEnhancer: LoudnessEnhancer? = null + internal var isAudioEffectSessionOpened = false + private set + internal var loudnessEnhancer: LoudnessEnhancer? = null + private set @@ - internal var discordRpc: DiscordRPC? = null + internal var discordRpc: DiscordRPC? = null + private set @@ - internal var scrobbleManager: ScrobbleManager? = null + internal var scrobbleManager: ScrobbleManager? = null + private setAlso applies to: 362-363, 365-369
🤖 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` at line 254, Several state properties (notably crossfadeEnabled in MusicService) were widened to mutable internal vars; tighten their visibility by keeping external read-only access and restricting writes with private setters. For each such property (e.g., crossfadeEnabled and the other internal var properties declared later in this file/class), change their declaration to keep internal visibility but add a private setter (use "private set") so callers can read but only the MusicService implementation can mutate them. Ensure you update any internal assignments remain valid inside the class.app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (1)
178-228: ActionCard component is well-structured.The
ActionCardcomposable provides a clean, reusable card with title, description, and action button. The destructive styling with error colors is appropriately applied for dangerous actions.Note: There's a similarly named
ActionCardinAboutScreen.kt(lines 244-296) with a different signature and layout. Consider whether these should be unified or if the naming collision is intentional given their different purposes.🤖 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 178 - 228, There are two different composables named ActionCard (the one here in ActionsPanel with signature ActionCard(title: String, description: String, buttonText: String, iconRes: Int, isDestructive: Boolean = false, onClick: () -> Unit) and the other ActionCard declared in AboutScreen) which causes a naming collision and confusion; fix it by either renaming one of them (e.g., ActionsPanelActionCard or AboutActionCard) and updating all call sites, or extracting shared UI into a single reusable composable and adjusting both usages to the unified function, ensuring you update imports/usages and keep the specific parameters/behavior (isDestructive, iconRes, layout) preserved.
🤖 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/ActionsPanel.kt`:
- Around line 108-109: The export currently writes raw log.message and
log.throwable (see ActionsPanel.kt writer.write call and symbols log.message,
log.throwable) which can leak secrets; add a redaction step (e.g., a
redactSensitiveData(String):String utility or method on
DevToolsLogBuffer/DevToolsTimberTree) and call it before writing/exporting logs
in ActionsPanel.kt (apply to both message and throwable.toString()), using
conservative regexes to mask bearer tokens, API keys, session ids, emails,
cookies, and any long hex/UUID-like secrets (replace matches with "[REDACTED]"
or masked substrings), ensure stack traces are scrubbed of file paths/params,
and reuse the same sanitizer anywhere logs are captured so export and in-memory
buffers share the redaction logic; include unit tests for the sanitizer to cover
common secret patterns.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Line 52: The playback-rate formatting uses String.format without a locale,
causing locale-dependent decimal separators; update the call that builds the
activity text (the expression combining song.song.title and
String.format("%.2fx", validPlaybackSpeed)) to use an explicit Locale (e.g.,
Locale.US) so the speed is always formatted with a dot (e.g.,
String.format(Locale.US, "%.2fx", validPlaybackSpeed)); locate the construction
in DiscordRPC.kt where song.song.title and validPlaybackSpeed are combined and
replace the implicit-format call accordingly.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt`:
- Around line 139-144: The cache-clear path computes pre-deletion size (sizeMb)
but ignores deleteRecursively() results, so the UI can show a success toast even
if deletion failed; update the block in ActionsPanel.kt that computes sizeMb and
performs deletion to capture each deletion result (e.g., collect boolean returns
from context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } or use
map/filter to determine any failures), recompute or verify post-deletion size
(or check aggregated boolean success) and only show the success Toast when
deletions succeeded, otherwise show a failure/error Toast with appropriate
message; reference the sizeMb variable and the deleteRecursively() calls when
making the changes.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`:
- Around line 119-123: The Y-axis bounds passed to coerceIn can be inverted when
2 * dragBoundsPaddingPx > screenHeightPx, causing coerceIn to throw; before
calling coerceIn for offsetY in the detectDragGestures lambda, compute
normalized minY and maxY (e.g., calculate rawMin = -screenHeightPx / 2f +
dragBoundsPaddingPx and rawMax = screenHeightPx / 2f - dragBoundsPaddingPx, then
if rawMin > rawMax set both to a safe fallback like the midpoint or the smaller
value) and call offsetY = (offsetY + dragAmount.y).coerceIn(minY, maxY) using
those normalized bounds to prevent IllegalArgumentException while keeping the
same variables (detectDragGestures, offsetY, dragBoundsPaddingPx,
screenHeightPx, coerceIn).
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 88-90: selectedLogIds can retain IDs that no longer exist after
log clears or ring-buffer rotations; update the selection whenever the backing
log list changes by pruning selectedLogIds to the intersection of current log
IDs (or clearing it when logs are emptied). In LogViewerPanel, add a derived
effect or observe the logs collection change (where selectedLogIds is declared
and around the other occurrence at lines ~344-346) and replace selectedLogIds
with selectedLogIds.filter { it in currentLogIds } (or set to emptySet() if
currentLogIds is empty) so the selected count always reflects existing rows.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt`:
- Around line 104-106: The UI is displaying raw enum .name for connectionState
and role (used in InfoRow calls) which bypasses localization; replace those uses
with localized strings by mapping each enum value to a string resource (either
via a when expression in PlayerStatePanel or small extension functions like
ConnectionState.toLocalizedString(context) and Role.toLocalizedString(context))
and pass stringResource(R.<...>) results to InfoRow instead of
connectionState.name and role.name, ensuring each enum branch maps to the
appropriate R.string resource for localization.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 405-408: Several call sites directly invoke discordRpc?.close()
inside coroutines (e.g., within scope.launch(Dispatchers.IO) and other shutdown
paths) which can throw and cancel surrounding coroutines; wrap each
discordRpc?.close() call in a non-throwing guard such as runCatching {
discordRpc?.close() }.onFailure { Timber.e(it, "Error closing Discord RPC") }
(or an equivalent try/catch that logs the Throwable and does not rethrow), and
apply this change to every place that calls discordRpc?.close() (including the
calls currently inside scope.launch and any lifecycle/shutdown methods) so
exceptions are logged but do not cancel service coroutines.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 57-58: remainingDuration can be negative causing invalid presence
endTime; clamp it to zero before adjusting for playback speed. In the DiscordRPC
code compute remainingDuration = max(0, song.song.duration * 1000L -
currentPlaybackTimeMillis) and then compute adjustedRemainingDuration =
(remainingDuration / validPlaybackSpeed).toLong(), ensuring you reference the
existing remainingDuration and adjustedRemainingDuration variables so the
presence endTime is never set to a past timestamp.
In `@app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt`:
- Line 46: The logs in ScrobbleManager (calls like onSongStart and the other
places logging metadata.title) expose raw track titles; update those
Timber.tag(...) calls to only run on dev builds (guard with BuildConfig.DEBUG or
a isDebuggable check) and never log the plain metadata.title — instead log a
non-identifying placeholder or a deterministic hash/obfuscated value (e.g.,
SHA-256 of metadata.title) so you can correlate events in dev without exposing
raw listening history; apply this change to the logging sites in ScrobbleManager
(the onSongStart and the other methods referencing metadata.title).
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt`:
- Around line 178-228: There are two different composables named ActionCard (the
one here in ActionsPanel with signature ActionCard(title: String, description:
String, buttonText: String, iconRes: Int, isDestructive: Boolean = false,
onClick: () -> Unit) and the other ActionCard declared in AboutScreen) which
causes a naming collision and confusion; fix it by either renaming one of them
(e.g., ActionsPanelActionCard or AboutActionCard) and updating all call sites,
or extracting shared UI into a single reusable composable and adjusting both
usages to the unified function, ensuring you update imports/usages and keep the
specific parameters/behavior (isDestructive, iconRes, layout) preserved.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Line 254: Several state properties (notably crossfadeEnabled in MusicService)
were widened to mutable internal vars; tighten their visibility by keeping
external read-only access and restricting writes with private setters. For each
such property (e.g., crossfadeEnabled and the other internal var properties
declared later in this file/class), change their declaration to keep internal
visibility but add a private setter (use "private set") so callers can read but
only the MusicService implementation can mutate them. Ensure you update any
internal assignments remain valid inside the class.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/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/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/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.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/res/values/values.xml
- app/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.kt
- app/src/main/kotlin/com/metrolist/music/App.kt
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsTimberTree.kt
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLog.kt
- app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt
- app/src/main/kotlin/com/metrolist/music/utils/Utils.kt
adrielGGmotion
force-pushed
the
feature/devtools
branch
from
March 2, 2026 22:13
4a9587d to
9924261
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (2)
158-162:⚠️ Potential issue | 🟡 MinorCache clear silently ignores deletion failures.
deleteRecursively()returnsfalseon failure, but the result is ignored. The UI will show success even if files remain, misleading the user.🛠️ Proposed fix
val sizeMb = withContext(Dispatchers.IO) { val size = context.cacheDir.walkTopDown().filter { it.isFile }.map { it.length() }.sum() - context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } + val allDeleted = context.cacheDir.listFiles()?.all { it.deleteRecursively() } ?: true + if (!allDeleted) { + throw IllegalStateException("Some cache files could not be deleted") + } size / 1024 / 1024 }🤖 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 158 - 162, The cache-clearing code uses context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } but ignores deleteRecursively()'s Boolean result, so UI shows success even if deletions fail; update the block around sizeMb (the withContext(Dispatchers.IO) lambda) to collect and check the return values of deleteRecursively() for each file (or directory), record any failures (e.g., count or list failed paths), and surface that outcome to the caller/UI (log the failures and/or return a flag/throw an exception) so the UI can show an error when deletes fail rather than always reporting success.
49-64:⚠️ Potential issue | 🔴 CriticalCritical: Regex patterns missing capture groups will crash at runtime.
Lines 52-55 define patterns without capture groups, but
replaceAll("$1=<REDACTED>")at line 61 references group 1. When any ofvisitorData,dataSyncId,SAPISID, or__Secure-*patterns match, Java will throwIndexOutOfBoundsException.🐛 Proposed fix - wrap key names in capture groups
private val SENSITIVE_PATTERNS = listOf( Pattern.compile("([Aa]uth|[Tt]oken)[=:]\\s*[\\w\\-]{10,}", Pattern.CASE_INSENSITIVE), Pattern.compile("(cookie|session)[=:]\\s*[\\w\\-]{10,}", Pattern.CASE_INSENSITIVE), - Pattern.compile("visitorData[=:]\\s*[\\w\\-]{20,}", Pattern.CASE_INSENSITIVE), - Pattern.compile("dataSyncId[=:]\\s*[\\w\\-]{20,}", Pattern.CASE_INSENSITIVE), - Pattern.compile("SAPISID[=:]\\s*[\\w\\-]{20,}", Pattern.CASE_INSENSITIVE), - Pattern.compile("__Secure-[A-Z]+[=:]\\s*[\\w\\-]{10,}", Pattern.CASE_INSENSITIVE), + Pattern.compile("(visitorData)[=:]\\s*[\\w\\-]{20,}", Pattern.CASE_INSENSITIVE), + Pattern.compile("(dataSyncId)[=:]\\s*[\\w\\-]{20,}", Pattern.CASE_INSENSITIVE), + Pattern.compile("(SAPISID)[=:]\\s*[\\w\\-]{20,}", Pattern.CASE_INSENSITIVE), + Pattern.compile("(__Secure-[A-Z]+)[=:]\\s*[\\w\\-]{10,}", Pattern.CASE_INSENSITIVE), )🤖 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 49 - 64, The regex list SENSITIVE_PATTERNS contains several patterns (e.g., the entries for visitorData, dataSyncId, SAPISID, __Secure-*) that do not define a capturing group, but redactSensitiveData uses replaceAll("$1=<REDACTED>") which will throw IndexOutOfBoundsException at runtime; fix by updating those Pattern.compile strings in SENSITIVE_PATTERNS to wrap the key portion in a capturing group (so group 1 is the key name) or change redactSensitiveData to use a replacement that doesn't rely on $1, ensuring the group reference matches the patterns; update SENSITIVE_PATTERNS entries and verify redactSensitiveData continues to call pattern.matcher(result).replaceAll("$1=<REDACTED>") accordingly.app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
83-97:⚠️ Potential issue | 🟡 MinorPrune
selectedLogIdswhen logs roll off the buffer.When the log buffer reaches capacity, old logs are overwritten but their IDs remain in
selectedLogIds. This causes the selection count to be inaccurate and the copy action to skip missing logs. Add aLaunchedEffectto prune stale selections.🛠️ Proposed fix
val onToggleLog = remember { { logId: Long -> val current = selectedLogIdsState.value selectedLogIdsState.value = if (logId in current) current - logId else current + logId } } + + LaunchedEffect(logs) { + val existingIds = logs.mapTo(HashSet()) { it.id } + val pruned = selectedLogIds.intersect(existingIds) + if (pruned.size != selectedLogIds.size) { + selectedLogIdsState.value = pruned + } + }🤖 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 83 - 97, Add a LaunchedEffect watching the collected logs (the logs variable from buffer.logs.collectAsState()) that prunes stale IDs out of selectedLogIdsState whenever the buffer rolls: compute the set of current log IDs (e.g. logs.map { it.id }.toSet()) and update selectedLogIdsState.value to selectedLogIdsState.value intersect that set so any IDs no longer present are removed; reference selectedLogIdsState and onToggleLog to locate selection state and toggle logic.app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
411-413:⚠️ Potential issue | 🟠 Major
discordRpc.close()is still unguarded in active cleanup paths.These close calls can still throw and cancel their parent coroutine/collector, which may interrupt service jobs unexpectedly. This concern was already raised previously and is still present.
🛠️ Hardening pattern
scope.launch(Dispatchers.IO) { - discordRpc?.close() + runCatching { discordRpc?.close() } + .onFailure { Timber.tag(TAG).e(it, "Discord RPC: close failed on screen-off cleanup") } }if (discordRpc?.isRpcRunning() == true) { Timber.tag(TAG).d("Discord RPC: tearing down previous instance") - discordRpc?.close() + runCatching { discordRpc?.close() } + .onFailure { Timber.tag(TAG).e(it, "Discord RPC: close failed during token refresh") } }scope.launch { - discordRpc?.close() + runCatching { discordRpc?.close() } + .onFailure { Timber.tag(TAG).e(it, "Discord RPC: close failed on playback-stop cleanup") } }Also applies to: 769-772, 2153-2155
🤖 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 411 - 413, The discordRpc?.close() calls inside the cleanup coroutines (e.g., the scope.launch(Dispatchers.IO) blocks in MusicService) are unguarded and can throw, cancelling their parent coroutine; wrap each close in a try/catch (or use runCatching) inside the launched coroutine (or run it in withContext(NonCancellable) then catch Throwable) and log any exception instead of letting it propagate so the close cannot cancel service jobs—update each occurrence (the calls at the shown scope.launch(Dispatchers.IO) block, and the other instances around lines with discordRpc?.close()) to use this guarded pattern and log the error via the service logger.
🧹 Nitpick comments (3)
app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt (1)
120-123: Consider edge case inmaskTokenfor tokens between 6-8 characters.For tokens with length 6-8, returning
"****"is correct. However, the threshold of<= 8means a 9-character token would show 6 characters (4 + 2 + "..."), which might be too revealing for very short tokens.Consider using a slightly higher threshold or a ratio-based approach for better security.
🔧 Optional improvement
fun maskToken(token: String): String { - if (token.length <= 8) return "****" + if (token.length <= 10) return "****" return "${token.take(4)}...${token.takeLast(2)}" }🤖 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 120 - 123, The maskToken function currently reveals 6 characters for 9-char tokens; update maskToken to avoid leaking too much for short tokens by raising the reveal threshold or using a ratio-based rule: inside maskToken, treat tokens shorter than or equal to a larger threshold (e.g., <= 10 or <= 12) as fully masked ("****"), or compute visible chars based on length (e.g., reveal min(4, length/3) at start and min(2, length/6) at end) so only sufficiently long tokens expose parts—adjust the conditional and the string construction in maskToken accordingly.app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt (1)
51-54: Use locale-aware formatting for displayed counts.
toString()works, but localized formatting (1,000vs1000) is easier to scan in diagnostics UIs.🤖 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 51 - 54, The UI is calling toString() on numeric counts in DatabaseInfoPanel; replace those with locale-aware formatted strings (e.g., use java.text.NumberFormat.getIntegerInstance(...) or NumberFormat.getInstance(Locale.getDefault()) and call format(songCount), format(albumCount), format(artistCount), format(playlistCount)) and pass the formatted strings into InfoRow so counts render with the user's locale grouping/decimal rules; update the four InfoRow invocations that currently use songCount.toString(), albumCount.toString(), artistCount.toString(), and playlistCount.toString().app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
363-374: Avoid exposing mutable service internals across the module.Exposing
LoudnessEnhancer,DiscordRPC, andScrobbleManagerasinternalproperties widens mutation surface and makes accidental lifecycle interference easier. Prefer exposing read-only snapshots/flags for DevTools instead of concrete mutable objects.♻️ Suggested direction
- internal var loudnessEnhancer: LoudnessEnhancer? = null - private set - - internal var discordRpc: DiscordRPC? = null - private set + private var loudnessEnhancer: LoudnessEnhancer? = null + private var discordRpc: DiscordRPC? = null + + internal val isLoudnessEnhancerActive: Boolean + get() = loudnessEnhancer != null + + internal val isDiscordRpcActive: Boolean + get() = discordRpc?.isRpcRunning() == true🤖 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 363 - 374, The MusicService exposes mutable internals (loudnessEnhancer, discordRpc, scrobbleManager) as internal vars which increases mutation surface; make these properties private and expose only safe read-only views/flags or minimal interfaces instead (e.g. keep isAudioEffectSessionOpened as internal val or provide a public/internal getter, replace loudnessEnhancer/discordRpc/scrobbleManager vars with private nullable properties and add read-only accessors or boolean/status snapshots and lifecycle-safe wrapper methods on MusicService to interact with them); update all call sites to use the new accessors/wrappers and ensure lifecycle management remains inside MusicService (refer to symbols: isAudioEffectSessionOpened, loudnessEnhancer, discordRpc, scrobbleManager, MusicService).
🤖 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/DatabaseInfoPanel.kt`:
- Around line 44-47: The UI currently hardcodes the database name and status
inside DatabaseInfoPanel using InfoCard and InfoRow; change this to perform a
lightweight runtime probe (e.g., a quick ping/query on your DB client or a
dedicated isConnected/checkHealth method) and store the result in a state that
the composable observes, then render the DB name and status from that state
instead of stringResource(R.string.dev_db_name) and
stringResource(R.string.dev_db_connected). Locate the composable that uses
InfoCard and InfoRow in DatabaseInfoPanel.kt, invoke the async/lightweight check
(rememberCoroutineScope/LaunchedEffect or the existing DI client) to set a
"connected" boolean and actual name, and switch the displayed label to
"Connected"/"Disconnected" (or an error message) based on that boolean so the
panel reflects real runtime connectivity.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`:
- Around line 123-124: The FAB is anchored to the bottom-right but offsetY is
constrained symmetrically; update the vertical bounds calculation used by
offsetY (the expression using screenHeightPx, dragBoundsPaddingPx, boundY and
the coerceIn call that sets offsetY) to mirror the asymmetric approach used for
X so the lower bound is zero and the upper bound allows upward drag (e.g.,
coerceIn(-boundY, 0) style) and also change the FAB alignment from
Alignment.CenterEnd to Alignment.BottomEnd where it’s set so the anchor matches
the new Y bounds (refer to offsetY, dragBoundsPaddingPx, screenHeightPx and the
Alignment.CenterEnd/BottomEnd usage).
In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`:
- Around line 389-393: The toast and tapCount reset are executed immediately
because coroutineScope.launch is non-blocking; move the Toast.makeText(...) and
tapCount = 0 inside the coroutine after the call to context.dataStore.edit so
they run only after persistence completes, and wrap the edit call in try/catch
to surface failures (show an error Toast on exception). Specifically update the
block that references DeveloperModeKey, context.dataStore.edit,
coroutineScope.launch and the tapCount variable so the success Toast runs after
a successful write and an error Toast is shown if the edit throws.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt`:
- Around line 158-162: The cache-clearing code uses
context.cacheDir.listFiles()?.forEach { it.deleteRecursively() } but ignores
deleteRecursively()'s Boolean result, so UI shows success even if deletions
fail; update the block around sizeMb (the withContext(Dispatchers.IO) lambda) to
collect and check the return values of deleteRecursively() for each file (or
directory), record any failures (e.g., count or list failed paths), and surface
that outcome to the caller/UI (log the failures and/or return a flag/throw an
exception) so the UI can show an error when deletes fail rather than always
reporting success.
- Around line 49-64: The regex list SENSITIVE_PATTERNS contains several patterns
(e.g., the entries for visitorData, dataSyncId, SAPISID, __Secure-*) that do not
define a capturing group, but redactSensitiveData uses
replaceAll("$1=<REDACTED>") which will throw IndexOutOfBoundsException at
runtime; fix by updating those Pattern.compile strings in SENSITIVE_PATTERNS to
wrap the key portion in a capturing group (so group 1 is the key name) or change
redactSensitiveData to use a replacement that doesn't rely on $1, ensuring the
group reference matches the patterns; update SENSITIVE_PATTERNS entries and
verify redactSensitiveData continues to call
pattern.matcher(result).replaceAll("$1=<REDACTED>") accordingly.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 83-97: Add a LaunchedEffect watching the collected logs (the logs
variable from buffer.logs.collectAsState()) that prunes stale IDs out of
selectedLogIdsState whenever the buffer rolls: compute the set of current log
IDs (e.g. logs.map { it.id }.toSet()) and update selectedLogIdsState.value to
selectedLogIdsState.value intersect that set so any IDs no longer present are
removed; reference selectedLogIdsState and onToggleLog to locate selection state
and toggle logic.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 411-413: The discordRpc?.close() calls inside the cleanup
coroutines (e.g., the scope.launch(Dispatchers.IO) blocks in MusicService) are
unguarded and can throw, cancelling their parent coroutine; wrap each close in a
try/catch (or use runCatching) inside the launched coroutine (or run it in
withContext(NonCancellable) then catch Throwable) and log any exception instead
of letting it propagate so the close cannot cancel service jobs—update each
occurrence (the calls at the shown scope.launch(Dispatchers.IO) block, and the
other instances around lines with discordRpc?.close()) to use this guarded
pattern and log the error via the service logger.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt`:
- Around line 51-54: The UI is calling toString() on numeric counts in
DatabaseInfoPanel; replace those with locale-aware formatted strings (e.g., use
java.text.NumberFormat.getIntegerInstance(...) or
NumberFormat.getInstance(Locale.getDefault()) and call format(songCount),
format(albumCount), format(artistCount), format(playlistCount)) and pass the
formatted strings into InfoRow so counts render with the user's locale
grouping/decimal rules; update the four InfoRow invocations that currently use
songCount.toString(), albumCount.toString(), artistCount.toString(), and
playlistCount.toString().
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 363-374: The MusicService exposes mutable internals
(loudnessEnhancer, discordRpc, scrobbleManager) as internal vars which increases
mutation surface; make these properties private and expose only safe read-only
views/flags or minimal interfaces instead (e.g. keep isAudioEffectSessionOpened
as internal val or provide a public/internal getter, replace
loudnessEnhancer/discordRpc/scrobbleManager vars with private nullable
properties and add read-only accessors or boolean/status snapshots and
lifecycle-safe wrapper methods on MusicService to interact with them); update
all call sites to use the new accessors/wrappers and ensure lifecycle management
remains inside MusicService (refer to symbols: isAudioEffectSessionOpened,
loudnessEnhancer, discordRpc, scrobbleManager, MusicService).
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 120-123: The maskToken function currently reveals 6 characters for
9-char tokens; update maskToken to avoid leaking too much for short tokens by
raising the reveal threshold or using a ratio-based rule: inside maskToken,
treat tokens shorter than or equal to a larger threshold (e.g., <= 10 or <= 12)
as fully masked ("****"), or compute visible chars based on length (e.g., reveal
min(4, length/3) at start and min(2, length/6) at end) so only sufficiently long
tokens expose parts—adjust the conditional and the string construction in
maskToken accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/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/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/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.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsTimberTree.kt
- app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt
- app/src/main/kotlin/com/metrolist/music/ui/screens/settings/SettingsScreen.kt
- app/src/main/res/values/values.xml
- app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt
- app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt
- app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLog.kt
Comment on lines
+44
to
+47
| InfoCard(title = stringResource(R.string.connection)) { | ||
| InfoRow(stringResource(R.string.database_name), stringResource(R.string.dev_db_name)) | ||
| InfoRow(stringResource(R.string.status), stringResource(R.string.dev_db_connected)) | ||
| } |
There was a problem hiding this comment.
Connection status is hardcoded, so diagnostics can be misleading.
The panel always shows Connected and a fixed DB name, even if runtime access is failing. Prefer deriving connection state from a lightweight real check/query result.
🤖 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 44 - 47, The UI currently hardcodes the database name and status
inside DatabaseInfoPanel using InfoCard and InfoRow; change this to perform a
lightweight runtime probe (e.g., a quick ping/query on your DB client or a
dedicated isConnected/checkHealth method) and store the result in a state that
the composable observes, then render the DB name and status from that state
instead of stringResource(R.string.dev_db_name) and
stringResource(R.string.dev_db_connected). Locate the composable that uses
InfoCard and InfoRow in DatabaseInfoPanel.kt, invoke the async/lightweight check
(rememberCoroutineScope/LaunchedEffect or the existing DI client) to set a
"connected" boolean and actual name, and switch the displayed label to
"Connected"/"Disconnected" (or an error message) based on that boolean so the
panel reflects real runtime connectivity.
Comment on lines
+123
to
+124
| val boundY = (screenHeightPx / 2f - dragBoundsPaddingPx).coerceAtLeast(0f) | ||
| offsetY = (offsetY + dragAmount.y).coerceIn(-boundY, boundY) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the file to understand its location and structure
fd "DevToolsOverlay.kt" -t fRepository: adrielGGmotion/Metrolist
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Read the file content around lines 123-128 to verify the issue
sed -n '100,150p' app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt | cat -nRepository: adrielGGmotion/Metrolist
Length of output: 2743
FAB is anchored at center-right, not bottom-right.
The FAB uses Alignment.CenterEnd on line 128, but the Y drag bounds on lines 124-125 should be adjusted to match a bottom-right anchor point. Currently, the Y bounds are symmetric (-boundY, boundY), which is appropriate for a center-anchored FAB. For a BottomEnd anchor, the bounds should be asymmetric like the X bounds, allowing dragging upward from the bottom position:
Suggested fix
- val boundY = (screenHeightPx / 2f - dragBoundsPaddingPx).coerceAtLeast(0f)
- offsetY = (offsetY + dragAmount.y).coerceIn(-boundY, boundY)
+ val minY = (-screenHeightPx + dragBoundsPaddingPx).coerceAtMost(0f)
+ offsetY = (offsetY + dragAmount.y).coerceIn(minY, 0f)Also change the alignment:
- .align(Alignment.CenterEnd)
+ .align(Alignment.BottomEnd)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val boundY = (screenHeightPx / 2f - dragBoundsPaddingPx).coerceAtLeast(0f) | |
| offsetY = (offsetY + dragAmount.y).coerceIn(-boundY, boundY) | |
| val minY = (-screenHeightPx + dragBoundsPaddingPx).coerceAtMost(0f) | |
| offsetY = (offsetY + dragAmount.y).coerceIn(minY, 0f) |
🤖 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 123 - 124, The FAB is anchored to the bottom-right but offsetY is
constrained symmetrically; update the vertical bounds calculation used by
offsetY (the expression using screenHeightPx, dragBoundsPaddingPx, boundY and
the coerceIn call that sets offsetY) to mirror the asymmetric approach used for
X so the lower bound is zero and the upper bound allows upward drag (e.g.,
coerceIn(-boundY, 0) style) and also change the FAB alignment from
Alignment.CenterEnd to Alignment.BottomEnd where it’s set so the anchor matches
the new Y bounds (refer to offsetY, dragBoundsPaddingPx, screenHeightPx and the
Alignment.CenterEnd/BottomEnd usage).
Comment on lines
+389
to
+393
| coroutineScope.launch { | ||
| context.dataStore.edit { it[DeveloperModeKey] = true } | ||
| } | ||
| Toast.makeText(context, context.getString(R.string.dev_mode_enabled), Toast.LENGTH_LONG).show() | ||
| tapCount = 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file
find . -name "AboutScreen.kt" -type fRepository: adrielGGmotion/Metrolist
Length of output: 144
🏁 Script executed:
# Read the file around lines 389-393
cat -n app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt | sed -n '380,410p'Repository: adrielGGmotion/Metrolist
Length of output: 2043
🏁 Script executed:
# Let's see more context to understand what coroutineScope is
cat -n app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt | sed -n '350,395p'Repository: adrielGGmotion/Metrolist
Length of output: 2884
🏁 Script executed:
# Let's see the function signature and imports to understand the context
cat -n app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt | sed -n '1,50p'Repository: adrielGGmotion/Metrolist
Length of output: 2709
🏁 Script executed:
# Let's find where coroutineScope is defined/used in this composable
cat -n app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt | sed -n '300,350p'Repository: adrielGGmotion/Metrolist
Length of output: 2427
Persist dev-mode before showing success feedback.
The Toast.makeText() call on line 392 executes immediately after launching the coroutine, not after the dataStore.edit completes. Since coroutineScope.launch is non-blocking, the success message is displayed before persistence is confirmed, and failures are not surfaced.
Move the toast and tapCount reset inside the coroutine scope to ensure they execute only after a successful write. Add error handling to surface failures to the user.
💡 Suggested fix
- coroutineScope.launch {
- context.dataStore.edit { it[DeveloperModeKey] = true }
- }
- Toast.makeText(context, context.getString(R.string.dev_mode_enabled), Toast.LENGTH_LONG).show()
- tapCount = 0
+ coroutineScope.launch {
+ 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,
+ e.message ?: "Failed to enable developer mode",
+ Toast.LENGTH_SHORT
+ ).show()
+ } finally {
+ tapCount = 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/ui/screens/settings/AboutScreen.kt`
around lines 389 - 393, The toast and tapCount reset are executed immediately
because coroutineScope.launch is non-blocking; move the Toast.makeText(...) and
tapCount = 0 inside the coroutine after the call to context.dataStore.edit so
they run only after persistence completes, and wrap the edit call in try/catch
to surface failures (show an error Toast on exception). Specifically update the
block that references DeveloperModeKey, context.dataStore.edit,
coroutineScope.launch and the tapCount variable so the success Toast runs after
a successful write and an error Toast is shown if the edit throws.
…tate, database info, and actions panel
adrielGGmotion
force-pushed
the
feature/devtools
branch
from
March 2, 2026 22:35
9924261 to
27ffb84
Compare
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive developer tools overlay, which is a significant and well-implemented feature for debugging. The changes include in-memory log capturing with a custom Timber tree, a circular log buffer, and a multi-panel UI for viewing logs, player state, database stats, and performing actions like clearing caches. The code is generally of high quality, leveraging dependency injection and modern Android practices. My review includes a few minor suggestions for improving code style, documentation consistency, and error reporting within the new developer tools.
Comment on lines
+161
to
+165
| var allDeleted = true | ||
| files?.forEach { if (!it.deleteRecursively()) allDeleted = false } | ||
| if (!allDeleted) { | ||
| // Some files couldn't be deleted, but we continue | ||
| } |
There was a problem hiding this comment.
The allDeleted flag is correctly updated if a file fails to delete, but its value is never used to inform the developer. If some files couldn't be deleted, it would be helpful to log a warning or show a more specific toast message to aid in debugging.
Suggested change
| var allDeleted = true | |
| files?.forEach { if (!it.deleteRecursively()) allDeleted = false } | |
| if (!allDeleted) { | |
| // Some files couldn't be deleted, but we continue | |
| } | |
| var allDeleted = true | |
| files?.forEach { if (!it.deleteRecursively()) allDeleted = false } | |
| if (!allDeleted) { | |
| timber.log.Timber.w("DevTools: Failed to delete some files from the cache directory.") | |
| } |
| 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 some version control systems.
}
}
Comment on lines
+117
to
+118
| * Masks a Discord token for safe logging. Shows only the first 4 and | ||
| * last 2 characters — enough to identify the token without leaking it. |
There was a problem hiding this comment.
The comment for maskToken states that it shows the 'first 4 and last 2 characters', but the implementation actually shows the first 3 and last 3. To avoid confusion, the comment should be updated to match the code's behavior.
Suggested change
| * Masks a Discord token for safe logging. Shows only the first 4 and | |
| * last 2 characters — enough to identify the token without leaking it. | |
| * Masks a Discord token for safe logging. Shows only the first 3 and | |
| * last 3 characters — enough to identify the token without leaking it. |
Owner
Author
|
@gemini-code-assist please focus on the mantainability of the code. Plus, look for possible peformance issues we can solve since |
|
Thanks for the clear instructions, @adrielGGmotion! I've reviewed the pull request with a focus on maintainability and potential performance issues, especially concerning Overall, this is a well-structured and highly valuable feature for debugging and understanding the application's runtime behavior. The modularization of the DevTools components and the use of dependency injection are good for maintainability. Performance Concerns
I've identified a potential performance issue related to In if (BuildConfig.DEBUG) {
Timber.plant(Timber.DebugTree())
}
Timber.plant(devToolsTimberTree) // This line is unconditionalThis means that even in release builds, and regardless of whether developer mode is enabled or the DevTools overlay is visible, the
To address this, Maintainability Improvements and Considerations
Let me know if you'd like me to elaborate on any of these points or if you have further questions! |
Owner
Author
|
@gemini-code-assist To finish, review the overall pull request and rate it now, that's all. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive developer tools overlay, which is a significant and well-implemented feature for debugging. The implementation includes in-memory logging, state inspection for playback and database, and various utility actions. The code is generally of high quality, with good practices like redacting sensitive information in logs. My review focuses on a few areas for improvement regarding data correctness in the UI, code style, and documentation accuracy. Overall, this is a great addition to the project.
Comment on lines
+97
to
+99
| val totalRamGb = memInfo.totalMem / 1024 / 1024 / 1024 | ||
| val availRamGb = memInfo.availMem / 1024 / 1024 / 1024 | ||
| val maxHeapMb = runtime.maxMemory() / 1024 / 1024 |
There was a problem hiding this comment.
The calculation for RAM sizes and heap memory uses integer division, which will truncate the results and display inaccurate values (e.g., 7.9 GB will show as 7 GB). To get precise values, you should use floating-point division.
Suggested change
| val totalRamGb = memInfo.totalMem / 1024 / 1024 / 1024 | |
| val availRamGb = memInfo.availMem / 1024 / 1024 / 1024 | |
| val maxHeapMb = runtime.maxMemory() / 1024 / 1024 | |
| val totalRamGb = memInfo.totalMem / (1024.0 * 1024 * 1024) | |
| val availRamGb = memInfo.availMem / (1024.0 * 1024 * 1024) | |
| val maxHeapMb = runtime.maxMemory() / (1024.0 * 1024.0) |
| } | ||
|
|
||
| @Composable | ||
| fun QueueViewerRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { |
There was a problem hiding this comment.
The fully qualified name for PlayerConnection is not necessary here. It's better to add an import for com.metrolist.music.playback.PlayerConnection at the top of the file and use the simple class name for improved readability.
Suggested change
| fun QueueViewerRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { | |
| fun QueueViewerRow(playerConnection: PlayerConnection) { |
| } | ||
|
|
||
| @Composable | ||
| fun TimelineRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { |
There was a problem hiding this comment.
The fully qualified name for PlayerConnection is not necessary here. It's better to add an import for com.metrolist.music.playback.PlayerConnection at the top of the file and use the simple class name for improved readability.
Suggested change
| fun TimelineRow(playerConnection: com.metrolist.music.playback.PlayerConnection) { | |
| fun TimelineRow(playerConnection: PlayerConnection) { |
| 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 and requirement for many linters to have a single newline at the end of every file.
Suggested change
| } | |
| } | |
Comment on lines
+117
to
+119
| * Masks a Discord token for safe logging. Shows only the first 4 and | ||
| * last 2 characters — enough to identify the token without leaking it. | ||
| */ |
There was a problem hiding this comment.
The documentation for the maskToken function is inconsistent with its implementation. The comment states it shows the first 4 and last 2 characters, but the code actually takes the first 3 and last 3. The comment should be updated to match the code's behavior.
Suggested change
| * Masks a Discord token for safe logging. Shows only the first 4 and | |
| * last 2 characters — enough to identify the token without leaking it. | |
| */ | |
| * Masks a Discord token for safe logging. Shows only the first 3 and | |
| * last 3 characters — enough to identify the token without leaking it. | |
| */ |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
389-393:⚠️ Potential issue | 🟡 MinorHandle dev-mode persistence failures and stop duplicate activation launches
At Line 389,
dataStore.edithas no failure path, and repeated taps before preference propagation can re-enter this branch and launch multiple writes/toasts. Reset/guard before launching, then wrap the write inrunCatching(ortry/catch) to surface failure.💡 Tight fix
remaining <= 0 -> { + tapCount = 0 coroutineScope.launch { - context.dataStore.edit { it[DeveloperModeKey] = true } - Toast.makeText(context, context.getString(R.string.dev_mode_enabled), Toast.LENGTH_LONG).show() - tapCount = 0 + runCatching { + context.dataStore.edit { it[DeveloperModeKey] = true } + }.onSuccess { + Toast.makeText( + context, + context.getString(R.string.dev_mode_enabled), + Toast.LENGTH_LONG + ).show() + }.onFailure { e -> + Toast.makeText( + context, + e.message ?: "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 389 - 393, The dev-mode activation branch spawns coroutineScope.launch which calls context.dataStore.edit and shows a Toast but lacks failure handling and allows duplicate launches if tapped repeatedly; before launching, set/guard a local flag or reset tapCount to prevent re-entry, then perform the write inside runCatching (or try/catch) around context.dataStore.edit( ... DeveloperModeKey = true ...) and on failure surface the error (e.g., via Timber/processLogger or a Toast) while only clearing tapCount and showing the success Toast on successful completion; reference coroutineScope.launch, context.dataStore.edit, DeveloperModeKey, tapCount, and Toast.makeText when applying the changes.app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt (1)
460-461:⚠️ Potential issue | 🟠 MajorSleep window fallback defaults are still inconsistent with Settings defaults.
At Line 460 and Line 461, fallback values remain
09:00-23:00, which can diverge from UI/default behavior when keys are missing.🔧 Proposed fix
- val sleepTimerStartTime = service.applicationContext.dataStore.get(SleepTimerStartTimeKey) ?: "09:00" - val sleepTimerEndTime = service.applicationContext.dataStore.get(SleepTimerEndTimeKey) ?: "23:00" + val sleepTimerStartTime = service.applicationContext.dataStore.get(SleepTimerStartTimeKey) ?: "22:00" + val sleepTimerEndTime = service.applicationContext.dataStore.get(SleepTimerEndTimeKey) ?: "06:00"🤖 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/PlayerConnection.kt` around lines 460 - 461, The code currently falls back to hardcoded "09:00" and "23:00" when reading SleepTimerStartTimeKey and SleepTimerEndTimeKey; replace those literals with the centralized defaults used by the Settings module so UI and logic remain consistent. Update the two lines that call service.applicationContext.dataStore.get(SleepTimerStartTimeKey) and get(SleepTimerEndTimeKey) to use the application-wide default constants (e.g., Settings.SLEEP_TIMER_DEFAULT_START and Settings.SLEEP_TIMER_DEFAULT_END or SleepTimerDefaults.START / SleepTimerDefaults.END) instead of "09:00"/"23:00", importing or referencing the Settings/SleepTimerDefaults symbols as needed.development_guide.md (1)
23-24:⚠️ Potential issue | 🟡 MinorAlign the APK output path with the build task.
Line 23buildsfossDebug, butLine 24still checksuniversalFoss, so this guide step is stale.Proposed doc fix
- ls app/build/outputs/apk/universalFoss/debug/app-universal-foss-debug.apk + ls app/build/outputs/apk/foss/debug/app-foss-debug.apk🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@development_guide.md` around lines 23 - 24, The APK path in the doc is out of sync with the Gradle task: the step runs ":app:assembleFossDebug" but then checks for "app-universal-foss-debug.apk"; update the guidance so both match by either changing the task to ":app:assembleUniversalFossDebug" to produce the universal artifact or, more simply, change the checked file name to the Foss debug artifact (e.g., the output produced by assembleFossDebug) so the path and the task are consistent; edit the lines mentioning ":app:assembleFossDebug" and "app-universal-foss-debug.apk" accordingly.app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt (1)
46-46:⚠️ Potential issue | 🟠 MajorGate track-metadata logs to dev-only capture paths.
Line 46,Line 54,Line 125, andLine 138log raw titles. With log capture/export in this PR, that can leak listening history and add avoidable hot-path logging overhead unless capture is strictly dev-only.#!/bin/bash # Verify Timber trees and DevTools log capture are gated for release safety. # Where Timber trees are planted rg -nP --type=kt '\bTimber\.plant\s*\(' -C3 # Where DevTools logging/export and gating signals are wired rg -nP --type=kt 'DevToolsTimberTree|DevToolsLogBuffer|export.*log|DeveloperModeKey|BuildConfig\.DEBUG' -C3Expected result: DevTools/log-export trees are gated by
BuildConfig.DEBUGand/or strict developer-mode checks, and not active in production paths.Also applies to: 54-54, 125-125, 138-138
🤖 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/ScrobbleManager.kt` at line 46, ScrobbleManager is directly logging raw track titles (e.g., Timber.tag(TAG).d("onSongStart: ${metadata.title}") and similar calls in the onSongStart/onSongEnd/update methods), which can leak listening history; update those log statements so they only emit the full metadata in dev builds or when a strict developer-mode flag is enabled (use BuildConfig.DEBUG or your DeveloperModeKey check), otherwise log a redacted/obfuscated placeholder (e.g., "onSongStart: <redacted>" or just metadata.id). Locate the Timber calls in ScrobbleManager (methods onSongStart, onSongEnd, and any update/state methods around the existing Timber lines) and wrap them in the feature gate or replace the interpolated ${metadata.title} with a non-sensitive token when not in dev mode.app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt (1)
104-106:⚠️ Potential issue | 🟡 MinorMap Listen Together enums to localized strings instead of using raw
.name.Line [104] and Line [106] currently expose enum constant names, which are not localized and can look internal/debuggy in UI. Please map both values through
when (...)to string resources before passing toInfoRow.🤖 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 104 - 106, The InfoRow calls are passing raw enum .name values (connectionState.name and role.name); replace those with localized strings by mapping the enums to stringResource via when expressions (e.g., switch on connectionState in PlayerStatePanel and return the appropriate stringResource(R.string.xxx) for each enum case, likewise for role), then pass the resulting localized string to InfoRow instead of .name; keep roomCode as-is.app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (1)
158-168:⚠️ Potential issue | 🟡 MinorDon’t silently report cache-clear success on deletion failures.
Line 161–165 tracks
allDeletedbut ignores it, so the success toast can still fire with leftover cache entries.🧹 Proposed fix
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 - } + val failed = context.cacheDir.listFiles() + ?.filterNot { it.deleteRecursively() } + .orEmpty() + if (failed.isNotEmpty()) { + throw IllegalStateException("Failed to delete ${failed.size} cache entries") + } size / 1024 / 1024 }🤖 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 158 - 168, ActionsPanel.kt currently computes sizeMb and attempts to delete cache files using context.cacheDir.walkTopDown()/deleteRecursively() but tracks deletions with allDeleted and then always shows the success Toast; change the flow in the coroutine that calculates sizeMb to record deletion success (use the existing allDeleted or compute failed items), and after the withContext block show a success Toast only when allDeleted is true otherwise show an error/warning Toast (or include the number/size of files that failed) so that delete failures are surfaced to the user; reference the variables/methods sizeMb, allDeleted, context.cacheDir.walkTopDown(), and deleteRecursively() to locate the logic to adjust.
🧹 Nitpick comments (3)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
371-395: Extract the tap activation state machine from the clickable lambdaThis block is doing timing, counting, UI messaging, and persistence dispatch all inline. Pulling it into a small helper (pure logic + callbacks) will make this Composable easier to read and much easier to unit test.
♻️ Suggested shape
- .clickable { - // full tap/countdown/persist logic - }, + .clickable { + handleDevModeTap( + nowMs = System.currentTimeMillis(), + isDeveloperModeEnabled = isDeveloperModeEnabled, + tapCount = tapCount, + lastTapTime = lastTapTime, + onTapCountChange = { tapCount = it }, + onLastTapTimeChange = { lastTapTime = it }, + onEnableDeveloperMode = { + coroutineScope.launch { context.dataStore.edit { it[DeveloperModeKey] = true } } + }, + onToast = { msg -> Toast.makeText(context, msg, 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 371 - 395, Extract the tap-activation logic out of the clickable lambda in AboutScreen.kt into a small helper (e.g., DeveloperTapController or handleDevTap function) that encapsulates state (lastTapTime, tapCount) and pure timing/counting logic using DEV_MODE_TAP_TIMEOUT_MS, DEV_MODE_REQUIRED_TAPS and DEV_MODE_COUNTDOWN_START and exposes a single method like onTap(): DevTapResult (or callbacks) indicating "show remaining X taps", "enabled", or "already enabled"; then call that helper from the clickable lambda and perform side effects (Toast, coroutineScope.dataStore.edit to set DeveloperModeKey) in the lambda based on the result—this keeps isDeveloperModeEnabled check, coroutineScope.launch and dataStore.edit only as side-effect handlers and makes the counting/timing logic unit-testable.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt (1)
51-59: Avoid the in-place “dead-end” state after turning dev mode off.When
Line 100switches dev mode off, recomposition hitsLine 51and returns early to a bare message view. Consider navigating back immediately (or keeping the top bar visible) for smoother UX.Possible adjustment
+import androidx.compose.runtime.LaunchedEffect @@ if (!devMode) { + LaunchedEffect(Unit) { + navController.navigateUp() + } Box( modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center ) { Text(stringResource(R.string.dev_mode_required)) } return }Also applies to: 95-101
🤖 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 51 - 59, DevToolsSettingsScreen currently returns early to a bare message when devMode is false, producing a dead-end UI after toggling dev mode off; update the composable so that when devMode becomes false you either navigate back (e.g., call navController.popBackStack() / navController.navigateUp() from the same place that currently returns) or remove the early return and render the normal scaffold/top bar instead (so the top bar remains visible). Locate the devMode check in DevToolsSettingsScreen and the place that flips devMode (the toggle handler around Line 100) and implement the navigation call or adjust the return behavior accordingly.app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLogBuffer.kt (1)
26-49: Per-log full snapshot rebuild can become a hot path.
add()(Line 40–49) emitsgetSnapshotLocked()every append, which isO(n)and allocates a new list per log. Under bursty logging this can create avoidable GC churn.Consider emitting append events (or batching snapshot emissions) and building/rendering the visible window on the UI side.
🤖 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/DevToolsLogBuffer.kt` around lines 26 - 49, The add() method in DevToolsLogBuffer currently calls getSnapshotLocked() on every append, causing O(n) work and allocations per log; instead modify the buffer to emit incremental append events or coalesce updates: change add(log: DevToolsLog) to push a lightweight append event (e.g., emit the single DevToolsLog or its index) to _logs (or a new _logEvents flow) and only occasionally (debounced/batched) call getSnapshotLocked() to publish the full snapshot for UI refresh; update consumers to handle incremental events and build the visible window client-side, keeping getSnapshotLocked() for full rebuilds when necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 39: The README line that documents the APK output after running
assembleFossDebug points to a non-existent `universalFoss` artifact; update the
APK path referenced for the `assembleFossDebug` build (mentioned near the
`assembleFossDebug` command) to the real output
`app/build/outputs/apk/foss/debug/app-foss-debug.apk` so the instruction matches
the generated APK location.
In `@app/src/main/kotlin/com/metrolist/music/App.kt`:
- Around line 74-77: DevToolsTimberTree is being planted unconditionally which
can cause memory/CPU overhead in production; update the App.kt initialization so
Timber.plant(devToolsTimberTree) only runs when appropriate (e.g., guard it with
BuildConfig.DEBUG or a check against the developer mode preference) and/or use
lazy planting that reads the developer-mode setting before planting; locate the
Timber.plant(devToolsTimberTree) call and wrap it with the conditional (using
BuildConfig.DEBUG or your developer-mode preference API) or delay planting until
developer mode is enabled to avoid allocating DevToolsLog objects and stack
traces in release builds.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/DevToolsOverlay.kt`:
- Around line 109-123: Saved offsetX/offsetY can be restored out-of-bounds after
configuration changes because you only clamp during drag; fix by clamping the
restored values with the same bounds logic before they are used for layout.
After the rememberSaveable declarations for offsetX and offsetY (or just before
constructing the Modifier.offset on the FloatingActionButton), recompute the
same minX and boundY (using screenWidthPx, screenHeightPx, dragBoundsPaddingPx)
and assign offsetX = offsetX.coerceIn(minX, 0f) and offsetY =
offsetY.coerceIn(-boundY, boundY) so the FAB is always on-screen; reference
offsetX, offsetY, rememberSaveable, FloatingActionButton and the existing
minX/boundY calculations when applying the clamp.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt`:
- Around line 209-218: The UI is doing the heavy serialization (filter +
joinToString) inside the IconButton onClick before switching contexts; move that
work into the coroutine so it runs off the main thread: inside the onClick start
scope.launch and compute the text by calling withContext(Dispatchers.Default) {
logs.filter { it.id in selectedLogIds }.joinToString(...) { ...use
logTimeFormatter/Instant etc. } } and then call clipboard.setClipEntry(...) on
the main dispatcher; reference the onClick lambda, logs, selectedLogIds,
logTimeFormatter, Instant, scope.launch and clipboard.setClipEntry and add the
necessary kotlinx.coroutines imports (Dispatchers, withContext).
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 777-778: Remove logging of token-derived values: the Timber.d call
that logs Discord RPC creation with DiscordRPC.maskToken(key) should be changed
to avoid emitting any token material. In MusicService where discordRpc is
constructed (discordRpc = DiscordRPC(this, key)), delete or replace the
Timber.tag(TAG).d(...) line with a non-secret message such as logging only that
Discord RPC is being initialized or that a token was provided (boolean/state)
without including the token or masked token; reference the TAG constant, the
DiscordRPC constructor usage, the discordRpc variable and the maskToken helper
when locating the code to update.
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 546-548: Multiple concurrent launches of
checkAndStartAutomaticSleepTimer from the scope.launch call can all pass the
isActive check and do redundant work before sleepTimer.start(); guard this
section with a single-flight Mutex: add a private Mutex (e.g., autoSleepMutex)
and wrap the body of checkAndStartAutomaticSleepTimer with
autoSleepMutex.withLock { ... } so only one coroutine performs the dataStore
read, date calculation, logging and calls sleepTimer.start(); ensure the lock is
released promptly and keep sleepTimer.start() outside or inside the lock based
on desired ordering to avoid redundant reads and logs.
In `@app/src/main/kotlin/com/metrolist/music/utils/DiscordRPC.kt`:
- Around line 117-123: The KDoc for maskToken says it should show the first 4
and last 2 characters but the implementation returns first 3 and last 3; update
the implementation inside fun maskToken(token: String) to match the doc by
returning the first 4 and last 2 characters (e.g., use token.take(4) and
token.takeLast(2)) and keep the short-token fallback unchanged (adjust the
length threshold if needed so very-short tokens still return the masked
fallback).
In `@app/src/main/res/values/metrolist_strings.xml`:
- Line 963: Replace the plain string resource named "selected_count" with a
<plurals> resource so pluralization/localization works correctly; create plurals
entries for at least "one" and "other" (using "%d Selected" or localized
variants) and then update call sites that currently use
getString(R.string.selected_count, count) to use
getResources().getQuantityString(R.plurals.selected_count, count, count) (or
Context.getResources().getQuantityString(...) / Resources.getQuantityString(...)
depending on where it’s used) so the correct plural form is chosen by Android.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt`:
- Around line 158-168: ActionsPanel.kt currently computes sizeMb and attempts to
delete cache files using context.cacheDir.walkTopDown()/deleteRecursively() but
tracks deletions with allDeleted and then always shows the success Toast; change
the flow in the coroutine that calculates sizeMb to record deletion success (use
the existing allDeleted or compute failed items), and after the withContext
block show a success Toast only when allDeleted is true otherwise show an
error/warning Toast (or include the number/size of files that failed) so that
delete failures are surfaced to the user; reference the variables/methods
sizeMb, allDeleted, context.cacheDir.walkTopDown(), and deleteRecursively() to
locate the logic to adjust.
In `@app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt`:
- Around line 104-106: The InfoRow calls are passing raw enum .name values
(connectionState.name and role.name); replace those with localized strings by
mapping the enums to stringResource via when expressions (e.g., switch on
connectionState in PlayerStatePanel and return the appropriate
stringResource(R.string.xxx) for each enum case, likewise for role), then pass
the resulting localized string to InfoRow instead of .name; keep roomCode as-is.
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 460-461: The code currently falls back to hardcoded "09:00" and
"23:00" when reading SleepTimerStartTimeKey and SleepTimerEndTimeKey; replace
those literals with the centralized defaults used by the Settings module so UI
and logic remain consistent. Update the two lines that call
service.applicationContext.dataStore.get(SleepTimerStartTimeKey) and
get(SleepTimerEndTimeKey) to use the application-wide default constants (e.g.,
Settings.SLEEP_TIMER_DEFAULT_START and Settings.SLEEP_TIMER_DEFAULT_END or
SleepTimerDefaults.START / SleepTimerDefaults.END) instead of "09:00"/"23:00",
importing or referencing the Settings/SleepTimerDefaults symbols as needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`:
- Around line 389-393: The dev-mode activation branch spawns
coroutineScope.launch which calls context.dataStore.edit and shows a Toast but
lacks failure handling and allows duplicate launches if tapped repeatedly;
before launching, set/guard a local flag or reset tapCount to prevent re-entry,
then perform the write inside runCatching (or try/catch) around
context.dataStore.edit( ... DeveloperModeKey = true ...) and on failure surface
the error (e.g., via Timber/processLogger or a Toast) while only clearing
tapCount and showing the success Toast on successful completion; reference
coroutineScope.launch, context.dataStore.edit, DeveloperModeKey, tapCount, and
Toast.makeText when applying the changes.
In `@app/src/main/kotlin/com/metrolist/music/utils/ScrobbleManager.kt`:
- Line 46: ScrobbleManager is directly logging raw track titles (e.g.,
Timber.tag(TAG).d("onSongStart: ${metadata.title}") and similar calls in the
onSongStart/onSongEnd/update methods), which can leak listening history; update
those log statements so they only emit the full metadata in dev builds or when a
strict developer-mode flag is enabled (use BuildConfig.DEBUG or your
DeveloperModeKey check), otherwise log a redacted/obfuscated placeholder (e.g.,
"onSongStart: <redacted>" or just metadata.id). Locate the Timber calls in
ScrobbleManager (methods onSongStart, onSongEnd, and any update/state methods
around the existing Timber lines) and wrap them in the feature gate or replace
the interpolated ${metadata.title} with a non-sensitive token when not in dev
mode.
In `@development_guide.md`:
- Around line 23-24: The APK path in the doc is out of sync with the Gradle
task: the step runs ":app:assembleFossDebug" but then checks for
"app-universal-foss-debug.apk"; update the guidance so both match by either
changing the task to ":app:assembleUniversalFossDebug" to produce the universal
artifact or, more simply, change the checked file name to the Foss debug
artifact (e.g., the output produced by assembleFossDebug) so the path and the
task are consistent; edit the lines mentioning ":app:assembleFossDebug" and
"app-universal-foss-debug.apk" accordingly.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLogBuffer.kt`:
- Around line 26-49: The add() method in DevToolsLogBuffer currently calls
getSnapshotLocked() on every append, causing O(n) work and allocations per log;
instead modify the buffer to emit incremental append events or coalesce updates:
change add(log: DevToolsLog) to push a lightweight append event (e.g., emit the
single DevToolsLog or its index) to _logs (or a new _logEvents flow) and only
occasionally (debounced/batched) call getSnapshotLocked() to publish the full
snapshot for UI refresh; update consumers to handle incremental events and build
the visible window client-side, keeping getSnapshotLocked() for full rebuilds
when necessary.
In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt`:
- Around line 371-395: Extract the tap-activation logic out of the clickable
lambda in AboutScreen.kt into a small helper (e.g., DeveloperTapController or
handleDevTap function) that encapsulates state (lastTapTime, tapCount) and pure
timing/counting logic using DEV_MODE_TAP_TIMEOUT_MS, DEV_MODE_REQUIRED_TAPS and
DEV_MODE_COUNTDOWN_START and exposes a single method like onTap(): DevTapResult
(or callbacks) indicating "show remaining X taps", "enabled", or "already
enabled"; then call that helper from the clickable lambda and perform side
effects (Toast, coroutineScope.dataStore.edit to set DeveloperModeKey) in the
lambda based on the result—this keeps isDeveloperModeEnabled check,
coroutineScope.launch and dataStore.edit only as side-effect handlers and makes
the counting/timing logic unit-testable.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/DevToolsSettingsScreen.kt`:
- Around line 51-59: DevToolsSettingsScreen currently returns early to a bare
message when devMode is false, producing a dead-end UI after toggling dev mode
off; update the composable so that when devMode becomes false you either
navigate back (e.g., call navController.popBackStack() /
navController.navigateUp() from the same place that currently returns) or remove
the early return and render the normal scaffold/top bar instead (so the top bar
remains visible). Locate the devMode check in DevToolsSettingsScreen and the
place that flips devMode (the toggle handler around Line 100) and implement the
navigation call or adjust the return behavior accordingly.
ℹ️ 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 (7)
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/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/utils/ScrobbleManager.kt (1)
lastfm/src/main/kotlin/com/metrolist/lastfm/LastFM.kt (1)
updateNowPlaying(119-139)
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/devtools/ui/PlayerStatePanel.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/devtools/ui/DatabaseInfoPanel.kt (1)
app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt (2)
InfoCard(19-34)InfoRow(36-52)
🪛 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 (27)
app/src/main/kotlin/com/metrolist/music/utils/Utils.kt (2)
10-10: No actionable issue on this import.
This import is consistent with the new logging call usage.
14-14: Duplicate concern: verify release-path logging is still guaranteed at Line 14.
This is the same risk previously flagged: if no Timber tree is planted outside debug,Timber.e(...)here may be a no-op in production. Please re-check current startup wiring before merge.#!/bin/bash set -euo pipefail echo "=== App/Application startup: Timber planting ===" fd -e kt 'App|Application' app/src/main/kotlin | while read -r f; do echo "--- $f ---" rg -n -C3 'onCreate\s*\(|BuildConfig\.DEBUG|Timber\.plant\s*\(' "$f" || true done echo "" echo "=== reportException + fallback checks ===" rg -n -C3 'fun reportException\s*\(|Timber\.e\s*\(|treeCount|forest\s*\(' app/src/main/kotlinExpected verification outcome:
- At least one tree planted for non-debug builds, or
- A fallback path when
Timber.treeCount == 0so exceptions are still recorded.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AboutScreen.kt (1)
317-344: Scaffold migration looks solidTop app bar, snackbar host insets, and content padding integration are clean here.
app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt (1)
56-57: Good async wiring for PlayerConnection initialization path.Making
scopeconstructor-injected keeps coroutine ownership explicit and test-friendly.app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt (1)
62-63: Nice wrapper exposure for database open-state.
Line 62–Line 63cleanly forwards state without changing lifecycle behavior.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/SettingsScreen.kt (1)
55-55: Dev-mode gating in Settings is implemented cleanly.The preference read + conditional item insertion are straightforward and maintainable.
Also applies to: 212-220
app/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.kt (1)
56-57: Route registration for DevTools is clear and consistent.The new destination is added cleanly with expected parameters.
Also applies to: 392-394
app/src/main/kotlin/com/metrolist/music/devtools/ui/SharedDevToolsUI.kt (1)
19-34: Good shared UI primitives for DevTools panels.These components are reusable and keep panel rendering consistent.
Also applies to: 36-52
app/src/main/kotlin/com/metrolist/music/db/DatabaseDao.kt (1)
1730-1740: DevTools count queries look solid and fit the panel use case.Nice addition: reactive
COUNT(*)flows keep the database stats panel live without extra polling.app/src/main/kotlin/com/metrolist/music/devtools/ui/PlayerStatePanel.kt (2)
137-149: Listener lifecycle handling is clean here.
DisposableEffect+ explicitremoveListeneron dispose is correctly wired and avoids leaks for queue observation.
204-208: Good sentinel normalization before timeline formatting.Nice guard for
C.TIME_UNSETprior to rendering timeline values.app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (2)
410-414: Guarded Discord close calls are a solid reliability improvement.Using
runCatchingwith explicit failure logs here makes cleanup much safer under transient RPC failures.Also applies to: 2154-2158
3129-3135: Destroy-path RPC teardown is now race-safe.Capturing
val rpc = discordRpcbefore nulling the field and closing asynchronously is the right lifecycle cleanup pattern.app/src/main/res/values/metrolist_strings.xml (1)
917-1044: DevTools string coverage is thorough and well-structured.This block is complete and gives good naming consistency across logs/player/env/db/tools panels.
app/src/main/res/values/values.xml (1)
8-11: LGTM!Clean addition of DevTools dimension resources. The values are sensible for FAB positioning and drag bounds constraints.
app/src/main/kotlin/com/metrolist/music/App.kt (1)
62-63: LGTM on the injection setup.The DI wiring for
DevToolsTimberTreeis correctly configured. The concern is with unconditional planting addressed in the previous comment.app/src/main/kotlin/com/metrolist/music/MainActivity.kt (2)
232-234: LGTM on DevToolsLogBuffer injection.Standard Hilt field injection—correctly set up for the DevTools feature.
1149-1154: DevToolsOverlay correctly gated behind developer mode.Good call rendering the overlay only when
devModeis enabled. This ensures the UI stays hidden for regular users while the feature remains accessible for debugging.app/src/main/kotlin/com/metrolist/music/di/AppModule.kt (1)
38-45: LGTM on DevTools DI providers.Both providers are correctly marked
@Singletonto ensure a single shared buffer and tree instance across the app. ThemaxSize = 1000for the log buffer is a reasonable default to prevent unbounded memory growth.AGENTS.md (2)
9-10: LGTM on branch naming convention updates.Shorter prefixes (
feat/,ref/) are cleaner and align with common conventions like Conventional Commits.
32-32: Good governance: version bump restriction.Reserving version bumps for the core team after manual review prevents accidental or unauthorized version changes. Solid policy addition.
app/src/main/kotlin/com/metrolist/music/devtools/ui/DatabaseInfoPanel.kt (2)
35-36: Database status is captured once and won't reflect runtime changes.The
remember {}blocks capturedbNameandisDbOpenat initial composition without any keys, so if the database state changes during the app's lifecycle, the panel won't update. For a DevTools diagnostic panel, this could be misleading.That said, if the database becomes unavailable, the app likely has bigger problems. This is a minor accuracy concern for diagnostics.
25-66: Overall panel structure looks solid.The composable cleanly separates concerns with
InfoCardandInfoRowprimitives, and the flow collection for counts will auto-update the UI. NumberFormat usage ensures proper localization.app/src/main/kotlin/com/metrolist/music/devtools/DevToolsTimberTree.kt (1)
10-22: Implementation is clean, but watch the allocation overhead.The tree correctly captures logs and forwards them to the buffer. A few observations:
stackTraceToString()on line 19 involves reflection and string building—fine for debug, but adds overhead if this tree is planted in release builds.- Every log creates a new
DevToolsLogobject withSystem.nanoTime()andSystem.currentTimeMillis()calls.Since this is a developer tool, this is acceptable when gated properly. Just ensure it's not running unconditionally in production (see comment on
App.kt).app/src/main/kotlin/com/metrolist/music/devtools/DevToolsLog.kt (1)
10-27: Clean data class design.Good use of default parameters for
idandtimestamp. ThepriorityLabelcomputed property nicely maps Android log levels to concise labels with a sensible"?"fallback for unknown priorities.app/src/main/kotlin/com/metrolist/music/devtools/ui/LogViewerPanel.kt (1)
147-152: Selection pruning on log updates is clean.The
LaunchedEffect(logs)intersection logic keepsselectedLogIdsconsistent with live rows and avoids stale UI state.app/src/main/kotlin/com/metrolist/music/devtools/ui/ActionsPanel.kt (1)
49-63: Redaction replacement safety fix looks good.Nice improvement ensuring regexes provide capture group 1 before
$1=<REDACTED>replacement.
|
|
||
| ```bash | ||
| ./gradlew :app:assembleuniversalFossDebug | ||
| ./gradlew :app:assembleFossDebug |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if AGENTS.md exists and read the relevant lines
git ls-files | grep -i agentsRepository: adrielGGmotion/Metrolist
Length of output: 77
🏁 Script executed:
# Read AGENTS.md to see the actual content
cat -n AGENTS.md | head -50Repository: adrielGGmotion/Metrolist
Length of output: 4034
🏁 Script executed:
# Find build.gradle files to check flavor configuration
find . -name "build.gradle*" -type f | head -20Repository: adrielGGmotion/Metrolist
Length of output: 330
🏁 Script executed:
# Read the app/build.gradle.kts to check flavor configuration
cat -n app/build.gradle.kts | head -150Repository: adrielGGmotion/Metrolist
Length of output: 5753
🏁 Script executed:
# Search for flavor configurations and APK output paths
rg -n "flavorDimensions|productFlavors|universalFoss|foss" app/build.gradle.kts -C 3Repository: adrielGGmotion/Metrolist
Length of output: 470
APK path in line 43 doesn't match the assembleFossDebug output.
The documented path references a non-existent universalFoss flavor. The actual APK output for assembleFossDebug is located at app/build/outputs/apk/foss/debug/app-foss-debug.apk. Update line 43 to point to the correct path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 39, The README line that documents the APK output after
running assembleFossDebug points to a non-existent `universalFoss` artifact;
update the APK path referenced for the `assembleFossDebug` build (mentioned near
the `assembleFossDebug` command) to the real output
`app/build/outputs/apk/foss/debug/app-foss-debug.apk` so the instruction matches
the generated APK location.
Comment on lines
+74
to
+77
| if (BuildConfig.DEBUG) { | ||
| Timber.plant(Timber.DebugTree()) | ||
| } | ||
| Timber.plant(devToolsTimberTree) |
There was a problem hiding this comment.
DevToolsTimberTree is planted unconditionally—potential performance concern on release builds.
The devToolsTimberTree is planted regardless of BuildConfig.DEBUG or whether developer mode is actually enabled. This means every log statement in the app will flow through this tree in production, causing:
- Memory pressure from accumulating logs in the circular buffer
- CPU overhead from creating
DevToolsLogobjects and string allocations - Stack trace string generation for any logged throwables
Consider gating the tree planting on developer mode or at minimum BuildConfig.DEBUG:
🛠️ Suggested approach
if (BuildConfig.DEBUG) {
Timber.plant(Timber.DebugTree())
+ Timber.plant(devToolsTimberTree)
}
-Timber.plant(devToolsTimberTree)Or, if the DevTools should work in release builds when developer mode is enabled, consider a lazy-planting approach that checks the developer mode preference before planting.
🤖 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 74 - 77,
DevToolsTimberTree is being planted unconditionally which can cause memory/CPU
overhead in production; update the App.kt initialization so
Timber.plant(devToolsTimberTree) only runs when appropriate (e.g., guard it with
BuildConfig.DEBUG or a check against the developer mode preference) and/or use
lazy planting that reads the developer-mode setting before planting; locate the
Timber.plant(devToolsTimberTree) call and wrap it with the conditional (using
BuildConfig.DEBUG or your developer-mode preference API) or delay planting until
developer mode is enabled to avoid allocating DevToolsLog objects and stack
traces in release builds.
Comment on lines
+109
to
+123
| var offsetX by rememberSaveable { mutableFloatStateOf(0f) } | ||
| var offsetY by rememberSaveable { mutableFloatStateOf(0f) } | ||
|
|
||
| Box(modifier = Modifier.fillMaxSize()) { | ||
| FloatingActionButton( | ||
| onClick = { isPanelExpanded = true }, | ||
| modifier = Modifier | ||
| .offset { IntOffset(offsetX.roundToInt(), offsetY.roundToInt()) } | ||
| .pointerInput(Unit) { | ||
| detectDragGestures { change, dragAmount -> | ||
| change.consume() | ||
| val minX = (-screenWidthPx + dragBoundsPaddingPx).coerceAtMost(0f) | ||
| offsetX = (offsetX + dragAmount.x).coerceIn(minX, 0f) | ||
| val boundY = (screenHeightPx / 2f - dragBoundsPaddingPx).coerceAtLeast(0f) | ||
| offsetY = (offsetY + dragAmount.y).coerceIn(-boundY, boundY) |
There was a problem hiding this comment.
Clamp saved FAB offsets after configuration changes.
Line 109 and Line 110 persist drag offsets, but clamping currently happens only during drag (Line 120–123). After rotation/window-size changes, restored offsets can place the FAB off-screen and unreachable.
🛠️ Proposed fix
import androidx.compose.runtime.Composable
+import androidx.compose.runtime.LaunchedEffect
@@
var isPanelExpanded by remember { mutableStateOf(false) }
var offsetX by rememberSaveable { mutableFloatStateOf(0f) }
var offsetY by rememberSaveable { mutableFloatStateOf(0f) }
+
+ LaunchedEffect(screenWidthPx, screenHeightPx, dragBoundsPaddingPx) {
+ val minX = (-screenWidthPx + dragBoundsPaddingPx).coerceAtMost(0f)
+ val boundY = (screenHeightPx / 2f - dragBoundsPaddingPx).coerceAtLeast(0f)
+ offsetX = offsetX.coerceIn(minX, 0f)
+ offsetY = offsetY.coerceIn(-boundY, boundY)
+ }🤖 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 109 - 123, Saved offsetX/offsetY can be restored out-of-bounds
after configuration changes because you only clamp during drag; fix by clamping
the restored values with the same bounds logic before they are used for layout.
After the rememberSaveable declarations for offsetX and offsetY (or just before
constructing the Modifier.offset on the FloatingActionButton), recompute the
same minX and boundY (using screenWidthPx, screenHeightPx, dragBoundsPaddingPx)
and assign offsetX = offsetX.coerceIn(minX, 0f) and offsetY =
offsetY.coerceIn(-boundY, boundY) so the FAB is always on-screen; reference
offsetX, offsetY, rememberSaveable, FloatingActionButton and the existing
minX/boundY calculations when applying the clamp.
Comment on lines
+209
to
+218
| IconButton(onClick = { | ||
| val logsToCopy = logs.filter { it.id in selectedLogIds } | ||
| val text = logsToCopy.joinToString("\n\n") { log -> | ||
| "${logTimeFormatter.format(Instant.ofEpochMilli(log.timestamp))} ${log.priorityLabel}/${log.tag}\n${log.message}${log.throwable?.let { "\n$it" } ?: ""}" | ||
| } | ||
| scope.launch { | ||
| clipboard.setClipEntry(androidx.compose.ui.platform.ClipEntry( | ||
| android.content.ClipData.newPlainText("logs", text) | ||
| )) | ||
| } |
There was a problem hiding this comment.
Move selected-log serialization off the main thread.
Line 210–213 does filtering + joinToString before coroutine context switch, so large selections can stall UI.
⚙️ Proposed fix
IconButton(onClick = {
- val logsToCopy = logs.filter { it.id in selectedLogIds }
- val text = logsToCopy.joinToString("\n\n") { log ->
- "${logTimeFormatter.format(Instant.ofEpochMilli(log.timestamp))} ${log.priorityLabel}/${log.tag}\n${log.message}${log.throwable?.let { "\n$it" } ?: ""}"
- }
scope.launch {
+ val ids = selectedLogIds
+ val text = withContext(Dispatchers.Default) {
+ logs.asSequence()
+ .filter { it.id in ids }
+ .joinToString("\n\n") { log ->
+ "${logTimeFormatter.format(Instant.ofEpochMilli(log.timestamp))} ${log.priorityLabel}/${log.tag}\n${log.message}${log.throwable?.let { "\n$it" } ?: ""}"
+ }
+ }
clipboard.setClipEntry(androidx.compose.ui.platform.ClipEntry(
android.content.ClipData.newPlainText("logs", text)
))
}
selectedLogIds = emptySet()
}) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| IconButton(onClick = { | |
| val logsToCopy = logs.filter { it.id in selectedLogIds } | |
| val text = logsToCopy.joinToString("\n\n") { log -> | |
| "${logTimeFormatter.format(Instant.ofEpochMilli(log.timestamp))} ${log.priorityLabel}/${log.tag}\n${log.message}${log.throwable?.let { "\n$it" } ?: ""}" | |
| } | |
| scope.launch { | |
| clipboard.setClipEntry(androidx.compose.ui.platform.ClipEntry( | |
| android.content.ClipData.newPlainText("logs", text) | |
| )) | |
| } | |
| IconButton(onClick = { | |
| scope.launch { | |
| val ids = selectedLogIds | |
| val text = withContext(Dispatchers.Default) { | |
| logs.asSequence() | |
| .filter { it.id in ids } | |
| .joinToString("\n\n") { log -> | |
| "${logTimeFormatter.format(Instant.ofEpochMilli(log.timestamp))} ${log.priorityLabel}/${log.tag}\n${log.message}${log.throwable?.let { "\n$it" } ?: ""}" | |
| } | |
| } | |
| clipboard.setClipEntry(androidx.compose.ui.platform.ClipEntry( | |
| android.content.ClipData.newPlainText("logs", text) | |
| )) | |
| } | |
| selectedLogIds = emptySet() | |
| }) { |
🤖 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 209 - 218, The UI is doing the heavy serialization (filter + joinToString)
inside the IconButton onClick before switching contexts; move that work into the
coroutine so it runs off the main thread: inside the onClick start scope.launch
and compute the text by calling withContext(Dispatchers.Default) { logs.filter {
it.id in selectedLogIds }.joinToString(...) { ...use logTimeFormatter/Instant
etc. } } and then call clipboard.setClipEntry(...) on the main dispatcher;
reference the onClick lambda, logs, selectedLogIds, logTimeFormatter, Instant,
scope.launch and clipboard.setClipEntry and add the necessary kotlinx.coroutines
imports (Dispatchers, withContext).
Comment on lines
+777
to
778
| Timber.tag(TAG).d("Discord RPC: creating instance (token=%s)", DiscordRPC.maskToken(key)) | ||
| discordRpc = DiscordRPC(this, key) |
There was a problem hiding this comment.
Do not log token-derived values (even masked) in service logs.
Line [777] still emits secret-derived data. For auth tokens, safest path is to log only presence/state, never token material.
🔐 Minimal safe change
- Timber.tag(TAG).d("Discord RPC: creating instance (token=%s)", DiscordRPC.maskToken(key))
+ Timber.tag(TAG).d("Discord RPC: creating instance")🤖 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 777 - 778, Remove logging of token-derived values: the Timber.d call that
logs Discord RPC creation with DiscordRPC.maskToken(key) should be changed to
avoid emitting any token material. In MusicService where discordRpc is
constructed (discordRpc = DiscordRPC(this, key)), delete or replace the
Timber.tag(TAG).d(...) line with a non-secret message such as logging only that
Discord RPC is being initialized or that a token was provided (boolean/state)
without including the token or masked token; reference the TAG constant, the
DiscordRPC constructor usage, the discordRpc variable and the maskToken helper
when locating the code to update.
Comment on lines
+546
to
+548
| scope.launch { | ||
| checkAndStartAutomaticSleepTimer() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate PlayerConnection.kt =="
fd 'PlayerConnection\.kt$'Repository: adrielGGmotion/Metrolist
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find SleepTimer implementation and start() semantics =="
fd '.*SleepTimer.*\.kt$' | head -20Repository: adrielGGmotion/Metrolist
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for checkAndStartAutomaticSleepTimer and related patterns =="
rg -n 'checkAndStartAutomaticSleepTimer|sleepTimer\.start|sleepTimer\.isActive' --type kt -B2 -A2 | head -100Repository: adrielGGmotion/Metrolist
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read PlayerConnection.kt around lines 546-548 =="
wc -l app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktRepository: adrielGGmotion/Metrolist
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read PlayerConnection.kt context (lines 530-560) =="
sed -n '530,560p' app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktRepository: adrielGGmotion/Metrolist
Length of output: 1033
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for checkAndStartAutomaticSleepTimer implementation =="
rg -n 'checkAndStartAutomaticSleepTimer|onPlayWhenReadyChanged' app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt -A5Repository: adrielGGmotion/Metrolist
Length of output: 848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read SleepTimer.kt to understand start() semantics =="
wc -l app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.ktRepository: adrielGGmotion/Metrolist
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read full SleepTimer.kt =="
cat -n app/src/main/kotlin/com/metrolist/music/playback/SleepTimer.ktRepository: adrielGGmotion/Metrolist
Length of output: 7326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read full checkAndStartAutomaticSleepTimer implementation =="
sed -n '444,480p' app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktRepository: adrielGGmotion/Metrolist
Length of output: 2296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check dataStore.get() to see if it has side effects =="
rg -n 'fun get|suspend fun get' app/src/main/kotlin -A3 | grep -i datastore -A5 | head -30Repository: adrielGGmotion/Metrolist
Length of output: 2616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if checkAndStartAutomaticSleepTimer has side effects beyond start() =="
sed -n '444,520p' app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktRepository: adrielGGmotion/Metrolist
Length of output: 4197
Reduce redundant auto-sleep checks on rapid play transitions.
When onPlayWhenReadyChanged fires repeatedly during paused→playing transitions, multiple coroutines can each pass the isActive check before any calls sleepTimer.start(). While sleepTimer.start() is idempotent (cancels the previous job), concurrent executions cause redundant dataStore reads, date calculations, and logging. A Mutex would prevent this wasted work:
🔧 Suggested single-flight guard (Mutex)
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
...
+ private val autoSleepCheckMutex = Mutex()
...
- scope.launch {
- checkAndStartAutomaticSleepTimer()
- }
+ scope.launch {
+ autoSleepCheckMutex.withLock {
+ checkAndStartAutomaticSleepTimer()
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scope.launch { | |
| checkAndStartAutomaticSleepTimer() | |
| } | |
| scope.launch { | |
| autoSleepCheckMutex.withLock { | |
| checkAndStartAutomaticSleepTimer() | |
| } | |
| } |
🤖 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/PlayerConnection.kt` around
lines 546 - 548, Multiple concurrent launches of
checkAndStartAutomaticSleepTimer from the scope.launch call can all pass the
isActive check and do redundant work before sleepTimer.start(); guard this
section with a single-flight Mutex: add a private Mutex (e.g., autoSleepMutex)
and wrap the body of checkAndStartAutomaticSleepTimer with
autoSleepMutex.withLock { ... } so only one coroutine performs the dataStore
read, date calculation, logging and calls sleepTimer.start(); ensure the lock is
released promptly and keep sleepTimer.start() outside or inside the lock based
on desired ordering to avoid redundant reads and logs.
Comment on lines
+117
to
+123
| * Masks a Discord token for safe logging. Shows only the first 4 and | ||
| * last 2 characters — enough to identify the token without leaking it. | ||
| */ | ||
| fun maskToken(token: String): String { | ||
| if (token.length <= 12) return "****" | ||
| return "${token.take(3)}...${token.takeLast(3)}" | ||
| } |
There was a problem hiding this comment.
Token-masking KDoc does not match implementation.
Line 117–Line 119 says “first 4 / last 2”, but Line 122 actually exposes first 3 / last 3.
Doc fix
- * Masks a Discord token for safe logging. Shows only the first 4 and
- * last 2 characters — enough to identify the token without leaking it.
+ * Masks a Discord token for safe logging. Shows only the first 3 and
+ * last 3 characters — enough to identify the token without leaking it.🤖 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 117
- 123, The KDoc for maskToken says it should show the first 4 and last 2
characters but the implementation returns first 3 and last 3; update the
implementation inside fun maskToken(token: String) to match the doc by returning
the first 4 and last 2 characters (e.g., use token.take(4) and
token.takeLast(2)) and keep the short-token fallback unchanged (adjust the
length threshold if needed so very-short tokens still return the masked
fallback).
| <string name="logs_subtitle">Real-time application logs</string> | ||
| <string name="search_logs">Search logs...</string> | ||
| <string name="clear_search">Clear search</string> | ||
| <string name="selected_count">%d Selected</string> |
There was a problem hiding this comment.
Use a plural resource for selected_count instead of a plain %d string.
Line [963] should be a <plurals> entry to keep grammar/localization correct across languages.
🌐 Suggested resource change
- <string name="selected_count">%d Selected</string>
+ <plurals name="selected_count">
+ <item quantity="one">%d selected</item>
+ <item quantity="other">%d selected</item>
+ </plurals>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="selected_count">%d Selected</string> | |
| <plurals name="selected_count"> | |
| <item quantity="one">%d selected</item> | |
| <item quantity="other">%d selected</item> | |
| </plurals> |
🤖 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 963, Replace the plain
string resource named "selected_count" with a <plurals> resource so
pluralization/localization works correctly; create plurals entries for at least
"one" and "other" (using "%d Selected" or localized variants) and then update
call sites that currently use getString(R.string.selected_count, count) to use
getResources().getQuantityString(R.plurals.selected_count, count, count) (or
Context.getResources().getQuantityString(...) / Resources.getQuantityString(...)
depending on where it’s used) so the correct plural form is chosen by Android.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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