Open
Conversation
## COMMITS for REFACTOR 01 implement multi-account status updates for Matrix don't update redundant (same) status msg. this avoids triggering excess client /sync reqs populate custom format box with default template for user convenience add placeholders as hints, infer scheme of homerserver if omitted consistent status string footer with preview and actual better handling of untrimmed inputs, HtTp:// case --- ## COMMITS for REFACTOR 02 show [time elapsed / total] in Matrix status updates (from shared Discord stats) more aggressively clear statuses if screen off/player paused add option to configure update interval for Matrix set default to 15 second updates for Matrix strings in xml. obscure password/token at all stages add custom xml image icon for Matrix logo, `[m]` use dedicated/translatable strings.xml resources only impl secure token storage, thread-safety, force HTTPS for Matrix reformat INTENT section code/braces/indentation add docstrings consolidate loop; separate clear() and update() methods add mutex, scope, log. tidy up some lints. improved concurrency flow within updateMatrix lifecycle management, conflict cleanup
|
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 Matrix presence integration: preference keys and account model, encrypted token store, MatrixRPC HTTP client, MusicService presence update/clear wiring, Matrix settings UI and navigation route, strings, drawable, and a security-crypto dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Prefs as DataStore Prefs
participant MS as MusicService
participant Store as MatrixTokenStore
participant MRPC as MatrixRPC Client
participant Matrix as Matrix Server
Prefs->>MS: MatrixAccounts/Enable/Format/Interval/Clear changed
MS->>MS: rebuild clients / start/stop update jobs
MS->>Store: getToken(homeserver,userId)
alt token present
Store-->>MS: accessToken
MS->>MRPC: create MatrixRPC(homeserver,userId,accessToken)
MS->>MS: add to matrixRpcClients
end
Note over MS,MRPC: Playback/network/screen events trigger updates
MS->>MRPC: updateSong(song, currentPosition, statusFormat, presence)
MRPC->>MRPC: resolveVariables & dedupe
MRPC->>Matrix: PUT /_matrix/client/v3/user/{userId}/presence/{presence}
Matrix-->>MRPC: HTTP response
MRPC-->>MS: success/failure logged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds Matrix presence support so Metrolist can publish “listening” status to a user’s Matrix homeserver, with corresponding settings UI and playback-driven updates in MusicService.
Changes:
- Introduces Matrix settings UI (accounts + status template) and navigation entry.
- Adds Matrix presence client (
MatrixRPC) and wires updates/teardown intoMusicService. - Adds new preference keys and user-facing strings for the integration.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/values/metrolist_strings.xml | Adds Matrix integration strings, status template strings, and an information warning. |
| app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt | Implements Matrix Client-Server presence update calls and template variable resolution. |
| app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt | Adds UI for enabling Matrix presence, managing accounts, and editing status format. |
| app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/IntegrationScreen.kt | Adds Matrix entry to integrations list. |
| app/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.kt | Adds navigation route for Matrix settings screen. |
| app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt | Creates/tears down Matrix clients and triggers presence updates based on playback and preference changes. |
| app/src/main/kotlin/com/metrolist/music/models/MatrixAccount.kt | Adds serializable model for storing Matrix accounts. |
| app/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.kt | Adds DataStore preference keys for Matrix enable/accounts/status format. |
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt (1)
19-24: Reuse a sharedOkHttpClientfor Matrix accounts.Each
MatrixRPCbuilds its own client, whileMusicServicerecreates these objects on settings/account changes. That needlessly multiplies connection pools and dispatcher threads, andclose()does not tear them down; pass an app-scoped client instead.🤖 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/MatrixRPC.kt` around lines 19 - 24, MatrixRPC currently constructs its own OkHttpClient per instance (constructor private val client: OkHttpClient = OkHttpClient()), which multiplies connection pools and dispatcher threads when MusicService recreates MatrixRPC on settings/account changes; change MatrixRPC to require an injected, app-scoped OkHttpClient (remove the default new OkHttpClient()), have callers (e.g., MusicService) pass a shared singleton client, and ensure MatrixRPC.close() does not call client.dispatcher().executorService().shutdownNow() or client.connectionPool().evictAll() on the shared client (or documents that it must not close the injected client).
🤖 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/playback/MusicService.kt`:
- Around line 2969-2979: The updates race because cancelling matrixUpdateJob
doesn't stop the blocking HTTP call in MatrixRPC.updateSong; fix by serializing
updates using a Mutex: add a shared Mutex (e.g., matrixRpcMutex: Mutex) and
replace the current launch/cancel flow in updateMatrixRPC with a scope.launch {
matrixRpcMutex.withLock { val clients = matrixRpcClients.toList();
clients.forEach { client -> client.updateSong(song, statusFormat).onFailure {
Timber.tag(TAG).w(it, "Matrix RPC update failed") } } } } so each update waits
for the previous HTTP call to finish and preserves ordering.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt`:
- Around line 75-77: The Matrix access token is being persisted insecurely:
remove accessToken from rememberSaveable and from the serialized accountsJson
preference and instead store only non-secret metadata (homeserver and userId) in
your DataStore; move all token reads/writes to a secure store (e.g., Android
Keystore / EncryptedSharedPreferences or EncryptedFile) via a dedicated helper
(e.g., MatrixTokenStore.saveToken/getToken/removeToken) and update the UI code
(the accessToken variable and any code that builds accountsJson) to read/write
the token through that secure helper so tokens are never written into plain
preferences or saved-state.
In `@app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt`:
- Around line 55-68: The code in MatrixRPC builds a URL into the local variable
url then proceeds to send an Authorization bearer token without verifying the
scheme; add an explicit HTTPS enforcement right after building url (before
constructing Request and before any use of accessToken) by checking the parsed
URL's scheme (e.g., ensure the HttpUrl from toHttpUrlOrNull() isHttps or the url
string starts with "https://") and return early from the runCatching block if
the check fails so no bearer token is sent over plain HTTP.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt`:
- Around line 19-24: MatrixRPC currently constructs its own OkHttpClient per
instance (constructor private val client: OkHttpClient = OkHttpClient()), which
multiplies connection pools and dispatcher threads when MusicService recreates
MatrixRPC on settings/account changes; change MatrixRPC to require an injected,
app-scoped OkHttpClient (remove the default new OkHttpClient()), have callers
(e.g., MusicService) pass a shared singleton client, and ensure
MatrixRPC.close() does not call
client.dispatcher().executorService().shutdownNow() or
client.connectionPool().evictAll() on the shared client (or documents that it
must not close the injected client).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47d9e728-d281-4b11-a3d8-b90d893b261a
📒 Files selected for processing (8)
app/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/models/MatrixAccount.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/integrations/IntegrationScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.ktapp/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.ktapp/src/main/res/values/metrolist_strings.xml
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
Outdated
Show resolved
Hide resolved
Collaborator
|
@gamesguru Never heard of that client but could i atleast see how this integration looks like at the "Matrix" app? |
Member
change the icon to the matrix icon |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
425-432:⚠️ Potential issue | 🟠 MajorSerialize Matrix close/update requests per client.
matrixUpdateJob?.cancel()does not stop the in-flight synchronousexecute()insideMatrixRPC.updateSong, andclient.close()runs outside that job altogether. A quick pause/resume or account refresh can therefore let an older offline request finish after a newer online one and leave Matrix presence stale; queue these operations behind one mutex/actor instead of cancel-and-relaunch.Also applies to: 849-857, 2308-2315, 2975-2985
🤖 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 425 - 432, The current cancel-and-close pattern around matrixUpdateJob and matrixRpcClients leaves in-flight synchronous MatrixRPC.updateSong.execute() calls running and allows client.close() to race with them; replace this with a per-client serialized queue (e.g., a coroutine actor or a Mutex + single coroutine) that accepts operations like UPDATE and CLOSE and executes them sequentially for each Matrix RPC client, stop sending new updates by posting a shutdown/close message to the actor instead of cancelling matrixUpdateJob directly, and await the actor's completion before returning; update the logic around matrixUpdateJob, MatrixRPC.updateSong invocations, and the loop over matrixRpcClients/client.close() (also apply same change to the other occurrences) so all update and close requests for a given client are executed in-order.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt (1)
75-77:⚠️ Potential issue | 🟠 MajorKeep Matrix access tokens out of saved state and plain preferences.
accessTokenis still held inrememberSaveableand serialized intoMatrixAccountsKey, so the secret lands in the saved-state bundle and plain DataStore. Persist only non-sensitive account metadata here and move the token to encrypted storage.Also applies to: 165-167, 188-195
🤖 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/integrations/MatrixSettings.kt` around lines 75 - 77, The accessToken is being stored in rememberSaveable (e.g., variable accessToken in MatrixSettings.kt) and thus serialized into MatrixAccountsKey and plain DataStore; stop saving sensitive tokens in saved-state or DataStore—keep only non-sensitive metadata (homeserver, userId) in rememberSaveable/MatrixAccountsKey and move token storage to encrypted storage (e.g., AndroidX Security: EncryptedSharedPreferences or an encrypted DataStore wrapper). Update code paths that read/write accessToken (including the instances around lines with accessToken and any save/load that touches MatrixAccountsKey) to instead read/write the token via the encrypted store API and ensure UI state uses a transient, non-saveable variable (e.g., remember without rememberSaveable) or fetch token on demand.
🤖 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/playback/MusicService.kt`:
- Around line 859-868: The code clears matrixRpcClients then decodes
accountsJson into List<MatrixAccount> but silently swallows exceptions; modify
the catch in the matrixEnabled branch to log the exception (including e and
accountsJson context) before returning emptyList(), e.g. use the existing logger
or processLogger to record a descriptive message about Matrix account decode
failure referencing matrixRpcClients/matrixEnabled/accountsJson/MatrixAccount so
operators see why Matrix presence was dropped.
- Around line 379-382: matrixRpcClients is accessed concurrently from multiple
coroutines (preference collector, updateMatrixRPC, and screen-state handlers)
causing race conditions; protect all reads and writes by introducing a single
Mutex (e.g. matrixRpcClientsMutex) and wrap every mutation and snapshot with
withLock { ... } (replace direct calls like matrixRpcClients.clear(), add(...),
and matrixRpcClients.toList() with guarded blocks), or alternatively confine all
access to a single dispatcher and switch callers to that dispatcher; update the
preference collector, updateMatrixRPC, and screen-state handler call sites so
every read/write uses the mutex-protected access to avoid
ConcurrentModificationExceptions and stale/duplicated clients.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt`:
- Around line 169-173: The current remember(accountsJson) block swallows JSON
decode errors by returning emptyList(), which hides malformed data and lets a
save overwrite it; instead, catch the exception from
matrixJsonSerializer.decodeFromString<List<MatrixAccount>>(accountsJson), log
the full error via your logger (or Timber) and surface a recovery state to the
UI (e.g., set an accountsParseError flag or provide a parsedAccounts nullable +
error message) so the UI can show an explicit "Restore/Reset/Import" option
rather than silently using emptyList(); update the code around the accounts
remember(...) block and any callers that consume accounts to handle the new
error state and avoid overwriting the original accountsJson until the user
chooses a recovery action.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 425-432: The current cancel-and-close pattern around
matrixUpdateJob and matrixRpcClients leaves in-flight synchronous
MatrixRPC.updateSong.execute() calls running and allows client.close() to race
with them; replace this with a per-client serialized queue (e.g., a coroutine
actor or a Mutex + single coroutine) that accepts operations like UPDATE and
CLOSE and executes them sequentially for each Matrix RPC client, stop sending
new updates by posting a shutdown/close message to the actor instead of
cancelling matrixUpdateJob directly, and await the actor's completion before
returning; update the logic around matrixUpdateJob, MatrixRPC.updateSong
invocations, and the loop over matrixRpcClients/client.close() (also apply same
change to the other occurrences) so all update and close requests for a given
client are executed in-order.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt`:
- Around line 75-77: The accessToken is being stored in rememberSaveable (e.g.,
variable accessToken in MatrixSettings.kt) and thus serialized into
MatrixAccountsKey and plain DataStore; stop saving sensitive tokens in
saved-state or DataStore—keep only non-sensitive metadata (homeserver, userId)
in rememberSaveable/MatrixAccountsKey and move token storage to encrypted
storage (e.g., AndroidX Security: EncryptedSharedPreferences or an encrypted
DataStore wrapper). Update code paths that read/write accessToken (including the
instances around lines with accessToken and any save/load that touches
MatrixAccountsKey) to instead read/write the token via the encrypted store API
and ensure UI state uses a transient, non-saveable variable (e.g., remember
without rememberSaveable) or fetch token on demand.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 847434b5-0f5c-4fe0-a96b-a8ae2b1723ce
📒 Files selected for processing (3)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.ktapp/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
Outdated
Show resolved
Hide resolved
gamesguru
force-pushed
the
guru/feat/matrix-status-updates
branch
from
84c4dab to
3cefa32
Compare
Author
@nyxiereal, please see updated screenshots on original post (above) and let me know if the aesthetics are acceptable. To me, the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (3)
2982-2982: Minor: Simplify boolean comparison.
dataStore.get(EnableMatrixRPCKey, false) != truecan be simplified to!dataStore.get(EnableMatrixRPCKey, false).🤖 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 2982, Replace the verbose boolean comparison in MusicService.kt by negating the stored value directly: locate the conditional using dataStore.get(EnableMatrixRPCKey, false) != true (inside the MusicService class or method handling RPC enabling) and change it to use the simpler form !dataStore.get(EnableMatrixRPCKey, false) so the condition reads as a direct negation of the retrieved boolean.
3038-3069: Consider releasing mutexes before network I/O.The
clearStatus()calls at line 3066 execute network requests while holding bothmatrixRpcUpdateMutexandmatrixRpcClientsMutex. With multiple accounts or slow networks, this blocks all other Matrix operations for extended periods.Consider snapshotting the client list and releasing locks before the network calls:
♻️ Suggested refactor
private fun clearMatrixRPC(showToast: Boolean = false) { matrixUpdateJob?.cancel() matrixUpdateJob = scope.launch { - var count = 0 - matrixRpcUpdateMutex.withLock { - matrixRpcClientsMutex.withLock { - // ... client loading logic ... - matrixRpcClients.forEach { it.clearStatus() } - count = matrixRpcClients.size - } + val clientsSnapshot: List<MatrixRPC> + matrixRpcUpdateMutex.withLock { + clientsSnapshot = matrixRpcClientsMutex.withLock { + // ... client loading logic (unchanged) ... + matrixRpcClients.toList() + } } + // Network calls outside locks + clientsSnapshot.forEach { it.clearStatus() } + val count = clientsSnapshot.size if (showToast && count > 0) { // ... toast logic ... } } }🤖 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 3038 - 3069, The code currently holds matrixRpcUpdateMutex and matrixRpcClientsMutex while calling network I/O via MatrixRPC.clearStatus(), which can block other operations; change this by snapshotting the clients list while holding the locks (read matrixRpcClients into an immutable list), then release both mutexes and iterate over the snapshot calling clearStatus() so network calls happen outside of matrixRpcUpdateMutex and matrixRpcClientsMutex; keep the existing logic that loads accounts into matrixRpcClients when it is empty but only use that to build the snapshot before releasing locks.
871-871: Minor: Redundant== truecomparison.The expression
dataStore.get(EnableMatrixRPCKey, false) == truecan be simplified to justdataStore.get(EnableMatrixRPCKey, false)since the return type is alreadyBoolean.🤖 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 871, In MusicService.kt update the conditional that reads dataStore.get(EnableMatrixRPCKey, false) == true to just use the Boolean result directly (dataStore.get(EnableMatrixRPCKey, false)); locate the if block in the MusicService class where that check is performed and remove the redundant "== true" comparison to simplify the expression and keep the same behavior.app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt (1)
19-19: Consider configuring explicit timeouts for the shared OkHttpClient.The
sharedOkHttpClientuses OkHttp's default timeouts (10s for connect/read/write). Other parts of the codebase (e.g.,ListenTogetherClient,OpenRouterService) configure explicit timeouts to handle slow or unresponsive servers more gracefully. While this may not cause immediate issues, adding explicit timeout configuration would improve resilience.♻️ Suggested improvement
+import java.util.concurrent.TimeUnit + -private val sharedOkHttpClient: OkHttpClient = OkHttpClient() +private val sharedOkHttpClient: OkHttpClient = OkHttpClient.Builder() + .connectTimeout(15, TimeUnit.SECONDS) + .readTimeout(15, TimeUnit.SECONDS) + .writeTimeout(15, TimeUnit.SECONDS) + .build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt` at line 19, The sharedOkHttpClient is created with default OkHttp timeouts; update the OkHttpClient builder used for sharedOkHttpClient to set explicit connect/read/write (and optionally call) timeouts to match project conventions (similar to ListenTogetherClient/OpenRouterService); locate the sharedOkHttpClient declaration and replace OkHttpClient() with a configured OkHttpClient.Builder().connectTimeout(...).readTimeout(...).writeTimeout(...).callTimeout(...).build() using sensible values consistent with other clients.app/src/main/kotlin/com/metrolist/music/utils/MatrixTokenStore.kt (1)
10-10: Consider using Timber for logging consistency.The codebase uses
Timberfor logging elsewhere (e.g.,MatrixRPC.kt,MusicService.kt). Usingandroid.util.Logdirectly is inconsistent and loses Timber's benefits (tree-based control, automatic tagging).♻️ Suggested change
-import android.util.Log +import timber.log.Timber // Then replace Log calls: -Log.e(TAG, "Failed to initialize EncryptedSharedPreferences", e) +Timber.tag(TAG).e(e, "Failed to initialize EncryptedSharedPreferences") -Log.w(TAG, "saveToken: encrypted storage unavailable, token not persisted") +Timber.tag(TAG).w("saveToken: encrypted storage unavailable, token not persisted")🤖 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/MatrixTokenStore.kt` at line 10, Replace android.util.Log usage in MatrixTokenStore with Timber to match project logging conventions: remove the import android.util.Log, add import timber.log.Timber, and update any Log.d/Log.e/... calls inside the MatrixTokenStore class or functions (e.g., within MatrixTokenStore methods that log tokens or errors) to use Timber.d/Timber.e (or Timber.tag(...) if custom tags are needed); ensure any exception messages passed to Log are forwarded to Timber.e(exception, ...) to preserve stack traces.
🤖 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/utils/MatrixRPC.kt`:
- Around line 152-158: In resolveVariables, guard against a negative or unknown
SongEntity.duration (defaults to -1) before passing it to makeTimeString: check
song.song.duration and if it's negative or zero, pass 0 (or an empty/placeholder
string) to makeTimeString for the "{total_time}" replacement instead of
multiplying a negative value by 1000L; update the "{total_time}" replacement in
resolveVariables to compute totalMs only when duration > 0 (e.g., totalMs =
song.song.duration * 1000L else 0) so makeTimeString never receives a negative
millisecond value.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Line 2982: Replace the verbose boolean comparison in MusicService.kt by
negating the stored value directly: locate the conditional using
dataStore.get(EnableMatrixRPCKey, false) != true (inside the MusicService class
or method handling RPC enabling) and change it to use the simpler form
!dataStore.get(EnableMatrixRPCKey, false) so the condition reads as a direct
negation of the retrieved boolean.
- Around line 3038-3069: The code currently holds matrixRpcUpdateMutex and
matrixRpcClientsMutex while calling network I/O via MatrixRPC.clearStatus(),
which can block other operations; change this by snapshotting the clients list
while holding the locks (read matrixRpcClients into an immutable list), then
release both mutexes and iterate over the snapshot calling clearStatus() so
network calls happen outside of matrixRpcUpdateMutex and matrixRpcClientsMutex;
keep the existing logic that loads accounts into matrixRpcClients when it is
empty but only use that to build the snapshot before releasing locks.
- Line 871: In MusicService.kt update the conditional that reads
dataStore.get(EnableMatrixRPCKey, false) == true to just use the Boolean result
directly (dataStore.get(EnableMatrixRPCKey, false)); locate the if block in the
MusicService class where that check is performed and remove the redundant "==
true" comparison to simplify the expression and keep the same behavior.
In `@app/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt`:
- Line 19: The sharedOkHttpClient is created with default OkHttp timeouts;
update the OkHttpClient builder used for sharedOkHttpClient to set explicit
connect/read/write (and optionally call) timeouts to match project conventions
(similar to ListenTogetherClient/OpenRouterService); locate the
sharedOkHttpClient declaration and replace OkHttpClient() with a configured
OkHttpClient.Builder().connectTimeout(...).readTimeout(...).writeTimeout(...).callTimeout(...).build()
using sensible values consistent with other clients.
In `@app/src/main/kotlin/com/metrolist/music/utils/MatrixTokenStore.kt`:
- Line 10: Replace android.util.Log usage in MatrixTokenStore with Timber to
match project logging conventions: remove the import android.util.Log, add
import timber.log.Timber, and update any Log.d/Log.e/... calls inside the
MatrixTokenStore class or functions (e.g., within MatrixTokenStore methods that
log tokens or errors) to use Timber.d/Timber.e (or Timber.tag(...) if custom
tags are needed); ensure any exception messages passed to Log are forwarded to
Timber.e(exception, ...) to preserve stack traces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 448e626b-5e94-4bfa-b85d-9deacd3914f8
📒 Files selected for processing (13)
app/build.gradle.ktsapp/generate_proto.shapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/models/MatrixAccount.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/integrations/IntegrationScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.ktapp/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.ktapp/src/main/kotlin/com/metrolist/music/utils/MatrixTokenStore.ktapp/src/main/res/drawable/matrix_icon.xmlapp/src/main/res/values/metrolist_strings.xmlgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (7)
- app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/IntegrationScreen.kt
- app/build.gradle.kts
- app/src/main/res/drawable/matrix_icon.xml
- app/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.kt
- gradle/libs.versions.toml
- app/src/main/res/values/metrolist_strings.xml
- app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/kotlin/com/metrolist/music/models/MatrixAccount.kt
- app/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.kt
Author
Certainly, @adrielGGmotion. Thanks for wanting to see some client screenshots! There are multiple clients. Since it's an open-source and federated system, there's no "one" standard. But here's a few good examples. Let me know if there's anything about the PR you'd like to see cleaned up. I'll still push minor edits, but it's taking more complete shape already. Web (
|
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… into guru/feat/matrix-status-updates-refactor-001
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt (2)
85-91:⚠️ Potential issue | 🟠 MajorDon't keep the Matrix access token in
rememberSaveable.
rememberSaveablepersists this secret outsideMatrixTokenStore, so the token can land in saved instance state instead of only in encrypted storage. Keep it in plainrememberstate and reload it fromMatrixTokenStorewhen the dialog opens.🔐 Minimal fix
- var accessToken by rememberSaveable { + var accessToken by remember(initialAccount?.homeserver, initialAccount?.userId) { mutableStateOf( initialAccount?.let { MatrixTokenStore.getToken(context, it.homeserver, it.userId) } ?: "" ) }🤖 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/integrations/MatrixSettings.kt` around lines 85 - 91, The access token is being stored in rememberSaveable (variable accessToken) which can persist secrets to saved instance state; change it to use plain remember instead and load the token from MatrixTokenStore.getToken(context, initialAccount.homeserver, initialAccount.userId) whenever the settings dialog opens (e.g., when initialAccount becomes non-null or on dialog entry) so the token remains only in memory and stays managed by MatrixTokenStore; update the MatrixSettings composable to replace rememberSaveable with remember and add a reload path that reads the token from MatrixTokenStore when the dialog is shown.
317-324:⚠️ Potential issue | 🟡 MinorDon't offer “Add Account” while the stored payload is in recovery mode.
When decoding fails,
accountsbecomes empty, so this branch still lets the user save a fresh list over the malformed JSON without explicitly choosing the reset action.🛠️ Simple guard
- if (accounts.size < 3) { + if (accountsParseError == null && accounts.size < 3) { accountItems.add( Material3SettingsItem( title = { Text(stringResource(R.string.matrix_add_account)) },🤖 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/integrations/MatrixSettings.kt` around lines 317 - 324, The "Add Account" entry is shown whenever accounts.size < 3 even if the stored payload decoding failed and you're in recovery mode; update the conditional that adds the Material3SettingsItem (the block referencing accounts, isAdding, Material3SettingsItem) to also check that the payload is not in recovery/decoded-failure state (e.g., add && !isRecoveryMode or && payloadDecodedSuccessfully) so the "Add Account" option is suppressed during recovery and users must explicitly perform the reset action instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt`:
- Around line 137-149: The confirmButton handler currently allows blank
homeserver, userId, or accessToken to be saved; update the onClick in
confirmButton to trim and validate inputs (homeserver -> normalizedHomeserver,
userId.trim(), accessToken.trim()) and only call onSave(MatrixAccount(...), ...)
when all three are non-blank; if any value is blank, abort the save (e.g.,
return early or disable the button) and surface an error/validation state so
incomplete credentials cannot be persisted. Ensure you reference the existing
symbols: confirmButton, homeserver, normalizedHomeserver, userId, accessToken,
onSave, and MatrixAccount.
- Around line 206-220: In onSave (lambda handling save flow) prevent creating
duplicate (homeserver, userId) entries before mutating accountsJson: check
accounts for any entry (use variable accounts) whose homeserver and userId match
the new account but whose index != editingIndex; if such a duplicate exists then
abort or surface an error instead of proceeding (do not call
MatrixTokenStore.saveToken or update accountsJson), otherwise continue with the
existing logic (remove old token for changed pair, save token via
MatrixTokenStore, update newAccounts and set accountsJson via
matrixJsonSerializer). Ensure the duplicate check covers both add (isAdding) and
edit paths and references MatrixAccount, editingIndex, isAdding,
MatrixTokenStore, accountsJson, and matrixJsonSerializer to locate the code.
---
Duplicate comments:
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt`:
- Around line 85-91: The access token is being stored in rememberSaveable
(variable accessToken) which can persist secrets to saved instance state; change
it to use plain remember instead and load the token from
MatrixTokenStore.getToken(context, initialAccount.homeserver,
initialAccount.userId) whenever the settings dialog opens (e.g., when
initialAccount becomes non-null or on dialog entry) so the token remains only in
memory and stays managed by MatrixTokenStore; update the MatrixSettings
composable to replace rememberSaveable with remember and add a reload path that
reads the token from MatrixTokenStore when the dialog is shown.
- Around line 317-324: The "Add Account" entry is shown whenever accounts.size <
3 even if the stored payload decoding failed and you're in recovery mode; update
the conditional that adds the Material3SettingsItem (the block referencing
accounts, isAdding, Material3SettingsItem) to also check that the payload is not
in recovery/decoded-failure state (e.g., add && !isRecoveryMode or &&
payloadDecodedSuccessfully) so the "Add Account" option is suppressed during
recovery and users must explicitly perform the reset action instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2e7719a-2c41-472b-9c46-4e1d3682ed9e
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt (2)
206-220:⚠️ Potential issue | 🟠 MajorReject duplicate
(homeserver, userId)entries before saving.
MatrixTokenStorekeys tokens by(homeserver, userId). Allowing two rows with the same identifiers means editing or deleting one can overwrite/remove the other's token.🛡️ Add duplicate check before mutating state
- onSave = { account, token -> + onSave = onSave@{ account, token -> + val duplicateIndex = accounts.indexOfFirst { + it.homeserver == account.homeserver && it.userId == account.userId + } + if (duplicateIndex != -1 && duplicateIndex != editingIndex) { + // TODO: Surface validation error to user + return@onSave + } + val oldAccount = editingIndex?.let { accounts.getOrNull(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/ui/screens/settings/integrations/MatrixSettings.kt` around lines 206 - 220, Before mutating state in the onSave handler, check for an existing account with the same homeserver and userId and reject the operation if found: in the onSave lambda (the block that uses editingIndex, isAdding, accounts, MatrixTokenStore, matrixJsonSerializer and sets accountsJson) add a duplicate check that scans accounts for an entry with matching homeserver and userId (when editing, ignore the current editingIndex), and if a duplicate exists abort the save (return/notify) without calling MatrixTokenStore.saveToken or modifying accountsJson; this prevents two rows from sharing the same (homeserver, userId) key and avoids token overwrite/removal issues.
137-149:⚠️ Potential issue | 🟠 MajorBlock saving incomplete account credentials.
The save button accepts blank homeserver, userId, or accessToken values and persists an account that cannot authenticate.
🛡️ Add validation to the save button
- TextButton( + TextButton( + enabled = + homeserver.isNotBlank() && + userId.isNotBlank() && + accessToken.isNotBlank(), onClick = { val trimmedServer = homeserver.trim()🤖 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/integrations/MatrixSettings.kt` around lines 137 - 149, The confirmButton's onClick currently trims and normalizes homeserver then unconditionally calls onSave with MatrixAccount(normalizedHomeserver, userId.trim()) and accessToken.trim(), allowing blank homeserver/userId/accessToken to be persisted; update the TextButton handler in confirmButton to validate that trimmed homeserver (after normalization), userId.trim(), and accessToken.trim() are non-blank before calling onSave (otherwise prevent saving and surface a validation error or keep the button disabled). Specifically, in the confirmButton/TextButton onClick for MatrixSettings, compute trimmedServer/userId/accessToken, derive normalizedHomeserver as now, then if any of normalizedHomeserver.isBlank(), userIdTrimmed.isBlank(), or accessTokenTrimmed.isBlank() return early (or set a local validation error state) instead of calling onSave(MatrixAccount(...), ...); ensure the same validation state is used to disable the save button when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.kt`:
- Around line 206-220: Before mutating state in the onSave handler, check for an
existing account with the same homeserver and userId and reject the operation if
found: in the onSave lambda (the block that uses editingIndex, isAdding,
accounts, MatrixTokenStore, matrixJsonSerializer and sets accountsJson) add a
duplicate check that scans accounts for an entry with matching homeserver and
userId (when editing, ignore the current editingIndex), and if a duplicate
exists abort the save (return/notify) without calling MatrixTokenStore.saveToken
or modifying accountsJson; this prevents two rows from sharing the same
(homeserver, userId) key and avoids token overwrite/removal issues.
- Around line 137-149: The confirmButton's onClick currently trims and
normalizes homeserver then unconditionally calls onSave with
MatrixAccount(normalizedHomeserver, userId.trim()) and accessToken.trim(),
allowing blank homeserver/userId/accessToken to be persisted; update the
TextButton handler in confirmButton to validate that trimmed homeserver (after
normalization), userId.trim(), and accessToken.trim() are non-blank before
calling onSave (otherwise prevent saving and surface a validation error or keep
the button disabled). Specifically, in the confirmButton/TextButton onClick for
MatrixSettings, compute trimmedServer/userId/accessToken, derive
normalizedHomeserver as now, then if any of normalizedHomeserver.isBlank(),
userIdTrimmed.isBlank(), or accessTokenTrimmed.isBlank() return early (or set a
local validation error state) instead of calling onSave(MatrixAccount(...),
...); ensure the same validation state is used to disable the save button when
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e18d1f4d-8c31-40d2-81f4-4cbeb5dcf7dd
📒 Files selected for processing (3)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/integrations/MatrixSettings.ktapp/src/main/kotlin/com/metrolist/music/utils/MatrixRPC.kt
Member
|
all fine and dandy about the icon, but i would like you to make this switch material 3 expressive 
look at omar's pr #3387 |
Author
|
@nyxiereal Okay, so it should grey-out (disable) and show a cross-out or "x" symbol? I think I have a few issues to clean up on this PR, the Also, there may be some unintended changes regarding "casting" or nefarious switching of the audio source. Unsure if it's something else on my end, but it ends up thinking it's casting (not in actual Bluetooth mode), which means i have to manually dig into my Bluetooth or volume/settings menu to adjust the volume. I'm also not completely sold on the slider UI or the fixed range of 5 to 120 or 180 seconds that I set for the update interval. Perhaps not the optimal user experience nor programming logic. |
Member
|
well, yeah (about the switch) good luck with the issues |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Lack of support for a common, open-source, and decentralized chat and discourse platform, called Matrix, often referred to by its most popular client UI, "Element" messenger.
Cause
As above.
Solution
Add missing service and helper logic/functions.
Testing
Iteratively and with multiple accounts,
but lightly with regard to status clearing, idling, unsetting.Edit: Logic for idle/background unsetting of status has been improved.
My emphasis on this PR was a stable UX and minimal polling logic, since matrix servers can be sensitive to unnecessary presence updates, and the Metrolist UI thread as well likely appreciates a philosophy of minimalism with regard to outgoing (often totally redundant) network requests/updates.
Screenshots
Summary by CodeRabbit
New Features
Behavior
Chores