feat(netease): stabilize cloud sync, rebuild restore, and pre-push test regressions#1104
Conversation
…st regressions - add full Netease bulk sync flow (playlists + songs) from dashboard sync action - repopulate Netease cloud songs into unified library after SyncWorker runs (including rebuild) - include `netease://` sources in Library ONLINE/OFFLINE filtering logic - prevent MediaStore deletion phase from removing cloud-backed songs - fix Netease song overwrite across playlists by using composite song keys - prune stale Netease playlists/songs that were removed remotely - update unit tests for changed method signatures and newly observed flows - keep `:app:compileDebugKotlin` and `:app:testDebugUnitTest` green
theovilardo
merged commit 25665c3
into
test/telegram-streaming-integration
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4b519050b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| albumArtist = null, | ||
| albumName = albumName, | ||
| albumId = albumId, | ||
| contentUriString = "netease://${neteaseSong.neteaseId}", |
There was a problem hiding this comment.
netease:// playback URIs
This writes Netease tracks into the unified library with a custom netease:// URI, but the playback resolver path still only special-cases Telegram URIs (DualPlayerEngine.resolveDataSpec checks dataSpec.uri.scheme == "telegram" and otherwise falls back to the default data source). As a result, synced Netease songs are discoverable in the library but fail at playback time because no component rewrites netease://... to an HTTP stream URL.
Useful? React with 👍 / 👎.
| // Persist cookies | ||
| prefs.edit().putString("netease_cookies", cookieJson).apply() | ||
| Timber.d("loginWithCookies: cookies saved to prefs") | ||
|
|
||
| // Initialize API client with cookies |
There was a problem hiding this comment.
Cookies are persisted and injected into the API client before user-account validation, so if getCurrentUserAccount() returns an error/expired session (no profile), the method returns failure but leaves invalid cookies saved. Because hasLogin() only checks whether MUSIC_U exists, subsequent app launches can report the user as logged in while every authenticated API call still fails until manual logout clears state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds first-class Netease Cloud Music integration (auth, API client/crypto, local proxy streaming, DB persistence, and UI entry points) and hooks it into the existing unified library + sync pipeline, while updating tests for signature/behavior changes.
Changes:
- Introduce Netease auth flow (WebView login), API client + encryption utilities, repository layer, and local Ktor proxy for streaming.
- Add Room tables/DAO + migration for Netease playlists/songs and unify them into the main library during sync.
- Update Home UI/navigation to surface a streaming provider chooser and Netease dashboard; adjust unit tests/mocks for updated dependencies.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/test/java/com/theveloper/pixelplay/presentation/viewmodel/PlayerViewModelTest.kt | Updates mocks/signatures for new storage filter + telegram deps. |
| app/src/test/java/com/theveloper/pixelplay/data/repository/MusicRepositoryImplTest.kt | Updates DAO stubs for updated getAlbums signature. |
| app/src/main/java/com/theveloper/pixelplay/presentation/screens/HomeScreen.kt | Adds provider sheet entry point; wires Netease dashboard navigation. |
| app/src/main/java/com/theveloper/pixelplay/presentation/netease/dashboard/NeteaseDashboardViewModel.kt | New VM for Netease dashboard state + sync actions. |
| app/src/main/java/com/theveloper/pixelplay/presentation/netease/dashboard/NeteaseDashboardScreen.kt | New Compose UI for Netease playlists and sync actions. |
| app/src/main/java/com/theveloper/pixelplay/presentation/netease/auth/NeteaseLoginViewModel.kt | New VM to process cookie-based login. |
| app/src/main/java/com/theveloper/pixelplay/presentation/netease/auth/NeteaseLoginActivity.kt | New WebView-based login activity + cookie capture. |
| app/src/main/java/com/theveloper/pixelplay/presentation/navigation/Screen.kt | Adds route for Netease dashboard. |
| app/src/main/java/com/theveloper/pixelplay/presentation/navigation/AppNavigation.kt | Registers Netease dashboard composable destination. |
| app/src/main/java/com/theveloper/pixelplay/presentation/components/StreamingProviderSheet.kt | New bottom sheet to choose Telegram vs Netease. |
| app/src/main/java/com/theveloper/pixelplay/di/Qualifiers.kt | Adds qualifier annotation for Netease Retrofit (future use). |
| app/src/main/java/com/theveloper/pixelplay/di/AppModule.kt | Adds Gson + NeteaseDao provider; includes DB migration 20→21. |
| app/src/main/java/com/theveloper/pixelplay/data/worker/SyncWorker.kt | Integrates Netease DB → unified library sync into worker. |
| app/src/main/java/com/theveloper/pixelplay/data/repository/MusicRepositoryImpl.kt | Updates storage filter documentation for cloud sources. |
| app/src/main/java/com/theveloper/pixelplay/data/network/netease/NeteaseModels.kt | Adds Netease API DTOs. |
| app/src/main/java/com/theveloper/pixelplay/data/network/netease/NeteaseEncryption.kt | Adds Netease request encryption utilities. |
| app/src/main/java/com/theveloper/pixelplay/data/network/netease/NeteaseApiService.kt | Adds OkHttp-based Netease API client + cookie handling. |
| app/src/main/java/com/theveloper/pixelplay/data/network/netease/CryptoMode.kt | Adds enum for Netease encryption modes. |
| app/src/main/java/com/theveloper/pixelplay/data/netease/NeteaseStreamProxy.kt | Adds local Ktor proxy to stream Netease audio to player. |
| app/src/main/java/com/theveloper/pixelplay/data/netease/NeteaseRepository.kt | Adds Netease persistence, sync, search, URL + lyrics resolution. |
| app/src/main/java/com/theveloper/pixelplay/data/model/Song.kt | Adds neteaseId field to Song model. |
| app/src/main/java/com/theveloper/pixelplay/data/database/SongEntity.kt | Parses neteaseId from contentUriString when mapping to Song. |
| app/src/main/java/com/theveloper/pixelplay/data/database/PixelPlayDatabase.kt | Adds Netease entities + migration 20→21 + DAO accessor. |
| app/src/main/java/com/theveloper/pixelplay/data/database/NeteaseSongEntity.kt | New Room entity + mapping helpers for Netease songs. |
| app/src/main/java/com/theveloper/pixelplay/data/database/NeteasePlaylistEntity.kt | New Room entity for Netease playlists. |
| app/src/main/java/com/theveloper/pixelplay/data/database/NeteaseDao.kt | New DAO for Netease playlists/songs. |
| app/src/main/java/com/theveloper/pixelplay/data/database/MusicDao.kt | Adds netease song cleanup + storage filter support for netease scheme. |
| app/src/main/AndroidManifest.xml | Registers NeteaseLoginActivity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| paddingValuesParent: PaddingValues, | ||
| playerViewModel: PlayerViewModel = hiltViewModel(), | ||
| settingsViewModel: SettingsViewModel = hiltViewModel(), | ||
| neteaseViewModel: NeteaseDashboardViewModel = hiltViewModel(), |
There was a problem hiding this comment.
HomeScreen injects NeteaseDashboardViewModel just to read isLoggedIn. This ViewModel currently auto-syncs in init, so opening the Home screen can trigger network/DB sync work even if the user never opens the Netease dashboard. Consider exposing login state via a lightweight VM/repository flow (or pass isLoggedIn in) and/or move the auto-sync trigger out of the ViewModel init (e.g., into the dashboard screen via LaunchedEffect) so it only runs when that screen is shown.
| neteaseViewModel: NeteaseDashboardViewModel = hiltViewModel(), | |
| neteaseViewModel: NeteaseDashboardViewModel, |
| val requestUrl = url.toHttpUrl() | ||
| Timber.d("$TAG: >>> $method $url [mode=$mode, persistedCookies=${usePersistedCookies}]") | ||
| Timber.d("$TAG: >>> params keys=${params.keys}") | ||
| Timber.d("$TAG: >>> hasLogin=${hasLogin()}, MUSIC_U=${persistedCookies["MUSIC_U"]?.take(20)}...") |
There was a problem hiding this comment.
NeteaseApiService logs the MUSIC_U cookie value (even partially). Session cookies are secrets and should never be written to logs (including debug logs), since they can be harvested from logcat/crash reports. Remove/obfuscate this entirely and log only non-sensitive diagnostics (e.g., hasLogin boolean).
| Timber.d("$TAG: >>> hasLogin=${hasLogin()}, MUSIC_U=${persistedCookies["MUSIC_U"]?.take(20)}...") | |
| Timber.d("$TAG: >>> hasLogin=${hasLogin()}, hasMusicUCookie=${persistedCookies.containsKey("MUSIC_U")}") |
| settings.apply { | ||
| javaScriptEnabled = true | ||
| domStorageEnabled = true | ||
| mixedContentMode = WebSettings.MIXED_CONTENT_ALWAYS_ALLOW |
There was a problem hiding this comment.
WebView is configured with MIXED_CONTENT_ALWAYS_ALLOW, which allows loading insecure HTTP subresources inside an HTTPS page. That weakens transport security and can enable MITM content injection. Prefer the default mixed content mode (or at least MIXED_CONTENT_COMPATIBILITY_MODE) unless there’s a proven requirement.
| mixedContentMode = WebSettings.MIXED_CONTENT_ALWAYS_ALLOW | |
| mixedContentMode = WebSettings.MIXED_CONTENT_COMPATIBILITY_MODE |
| webViewClient = WebViewClient() | ||
| loadUrl(NeteaseLoginActivity.TARGET_URL) | ||
| onWebViewCreated(this) | ||
| } |
There was a problem hiding this comment.
The WebView instance created in AndroidView isn’t explicitly destroyed when the composable leaves composition, which can leak the Activity/context and keep resources alive. Consider using AndroidView’s onRelease/onReset (or DisposableEffect) to call webView.stopLoading()/clearHistory()/removeAllViews()/destroy() and to clear the stored webView reference.
| } | |
| } | |
| }, | |
| onRelease = { webView -> | |
| webView.stopLoading() | |
| webView.clearHistory() | |
| webView.removeAllViews() | |
| webView.destroy() |
| * Netease Cloud Music crypto utility. | ||
| * Supports all 4 encryption modes used by the API. | ||
| * | ||
| * Reference: NeriPlayer's NeteaseCrypto.kt (GPL-3.0) |
There was a problem hiding this comment.
This file states it is based on/References a GPL-3.0 implementation. This repository is MIT-licensed, so incorporating GPL-derived code would impose GPL obligations and can make the combined work incompatible for distribution. Please confirm provenance and licensing; if it’s derived from GPL code, re-implement from a permissive source/spec or obtain a compatible license.
| * Reference: NeriPlayer's NeteaseCrypto.kt (GPL-3.0) | |
| * Implementation independently developed based on publicly available | |
| * descriptions of the Netease Cloud Music API; no GPL-licensed code | |
| * has been copied into this file. |
| // Persist cookies | ||
| prefs.edit().putString("netease_cookies", cookieJson).apply() | ||
| Timber.d("loginWithCookies: cookies saved to prefs") |
There was a problem hiding this comment.
Netease session cookies are persisted in plain SharedPreferences. These cookies function as credentials; storing them unencrypted increases exposure (e.g., on rooted devices, backups, or other local extraction). Consider using EncryptedSharedPreferences (or another secure storage) for netease_cookies and related auth state.
| import androidx.compose.ui.text.font.FontWeight | ||
| import androidx.compose.ui.text.style.TextAlign | ||
| import androidx.compose.ui.unit.dp | ||
| import com.theveloper.pixelplay.data.netease.NeteaseRepository |
There was a problem hiding this comment.
Unused import: NeteaseRepository is imported but not referenced in this file. Removing it will avoid lint warnings and keep imports clean.
| import com.theveloper.pixelplay.data.netease.NeteaseRepository |
| fun loadPlaylistSongs(playlistId: Long) { | ||
| viewModelScope.launch { | ||
| repository.getPlaylistSongs(playlistId).collect { songs -> | ||
| _selectedPlaylistSongs.value = songs | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
loadPlaylistSongs() launches a new collection every time it’s called and never cancels the previous collector. Re-selecting playlists will accumulate active collectors and keep doing work. Consider keeping a selectedPlaylistId StateFlow and using flatMapLatest/collectLatest, or track/cancel a Job before starting a new collection.
| val body = response.body | ||
|
|
||
| if (body == null) { | ||
| call.respond(HttpStatusCode.BadGateway, "No response body") | ||
| return@get | ||
| } | ||
|
|
||
| val contentLength = response.header("Content-Length") | ||
| val contentRange = response.header("Content-Range") | ||
| val acceptRanges = response.header("Accept-Ranges") | ||
|
|
||
| if (response.code == 206) { | ||
| call.response.status(HttpStatusCode.PartialContent) | ||
| } | ||
| call.response.header("Accept-Ranges", acceptRanges ?: "bytes") | ||
| contentLength?.let { call.response.header("Content-Length", it) } | ||
| contentRange?.let { call.response.header("Content-Range", it) } | ||
|
|
||
| call.respondBytesWriter(contentType = ContentType.Audio.Any) { | ||
| withContext(Dispatchers.IO) { | ||
| body.byteStream().use { input -> | ||
| val buffer = ByteArray(64 * 1024) | ||
| var bytesRead: Int | ||
| while (input.read(buffer).also { bytesRead = it } != -1) { | ||
| writeFully(buffer, 0, bytesRead) |
There was a problem hiding this comment.
The OkHttp Response returned by execute() is never closed. This will leak connections and eventually break streaming (connection pool exhaustion). Wrap the response in use { ... } (and ensure the body stream is fully consumed/closed) before returning from the Ktor handler.
| val body = response.body | |
| if (body == null) { | |
| call.respond(HttpStatusCode.BadGateway, "No response body") | |
| return@get | |
| } | |
| val contentLength = response.header("Content-Length") | |
| val contentRange = response.header("Content-Range") | |
| val acceptRanges = response.header("Accept-Ranges") | |
| if (response.code == 206) { | |
| call.response.status(HttpStatusCode.PartialContent) | |
| } | |
| call.response.header("Accept-Ranges", acceptRanges ?: "bytes") | |
| contentLength?.let { call.response.header("Content-Length", it) } | |
| contentRange?.let { call.response.header("Content-Range", it) } | |
| call.respondBytesWriter(contentType = ContentType.Audio.Any) { | |
| withContext(Dispatchers.IO) { | |
| body.byteStream().use { input -> | |
| val buffer = ByteArray(64 * 1024) | |
| var bytesRead: Int | |
| while (input.read(buffer).also { bytesRead = it } != -1) { | |
| writeFully(buffer, 0, bytesRead) | |
| response.use { resp -> | |
| val body = resp.body | |
| if (body == null) { | |
| call.respond(HttpStatusCode.BadGateway, "No response body") | |
| return@get | |
| } | |
| val contentLength = resp.header("Content-Length") | |
| val contentRange = resp.header("Content-Range") | |
| val acceptRanges = resp.header("Accept-Ranges") | |
| if (resp.code == 206) { | |
| call.response.status(HttpStatusCode.PartialContent) | |
| } | |
| call.response.header("Accept-Ranges", acceptRanges ?: "bytes") | |
| contentLength?.let { call.response.header("Content-Length", it) } | |
| contentRange?.let { call.response.header("Content-Range", it) } | |
| call.respondBytesWriter(contentType = ContentType.Audio.Any) { | |
| withContext(Dispatchers.IO) { | |
| body.byteStream().use { input -> | |
| val buffer = ByteArray(64 * 1024) | |
| var bytesRead: Int | |
| while (input.read(buffer).also { bytesRead = it } != -1) { | |
| writeFully(buffer, 0, bytesRead) | |
| } |
| Timber.d("loginWithCookies: starting, json length=${cookieJson.length}") | ||
| val cookies = jsonToMap(cookieJson) | ||
| Timber.d("loginWithCookies: parsed ${cookies.size} cookies, keys=${cookies.keys}") | ||
|
|
||
| if (!cookies.containsKey("MUSIC_U")) { | ||
| Timber.w("loginWithCookies: MUSIC_U not found in cookies!") | ||
| return@withContext Result.failure(Exception("MUSIC_U cookie not found")) | ||
| } | ||
| Timber.d("loginWithCookies: MUSIC_U found (${cookies["MUSIC_U"]?.take(20)}...)") | ||
|
|
There was a problem hiding this comment.
loginWithCookies() logs the MUSIC_U cookie value and other potentially sensitive account details. Authentication cookies/tokens should not be logged, since logs can be exported via bug reports/crash reports. Remove these logs or ensure they never include secrets/PII.
netease://sources in Library ONLINE/OFFLINE filtering logic:app:compileDebugKotlinand:app:testDebugUnitTestgreen